[PATCHv2 0/8] HFP_GW parser

[PATCHv2 0/8] HFP_GW parser

[Marcin Kraglak]

v2:
 - parser moved to src/shared/hfp
 - dynamic register/unregister prefix handlers
 - minor fixes pointed by Marcel

Marcin Kraglak (8):
  shared/hfp: Fix parsing line with few commands
  shared/hfp: Add prefix handlers functionality
  shared/hfp: Add initial implementiation of processing commands
  shared/hfp: Add implementation of precessing commands
  shared/hfp: Add get_number implementation
  shared/hfp: Add open/close container function
  shared/hfp: Add function to get string
  shared/hfp: Add function to get unquoted string

 Makefile.tools   |   1 +
 src/shared/hfp.c | 363 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 src/shared/hfp.h |  25 ++++
 3 files changed, 380 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCHv2 1/8] shared/hfp: Fix parsing line with few commands

[Marcin Kraglak]

If hfp_gw received line with few commands, it called command
handler one time, for first command. Next comamnd was processed
only if next line was received.
Now, after every response from gw, we call proces_input to be
sure that all data has been be processed.
It will process next command only if response for previous was sent.
---
 src/shared/hfp.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 854cf46..0681b19 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -121,23 +121,21 @@ static void process_input(struct hfp_gw *hfp)
 
 		*ptr = '\0';
 		count = asprintf(&ptr, "%s%s", str, str2);
-		str = ptr;
 	} else {
-		count = ptr - str;
 		*ptr = '\0';
+		count = asprintf(&ptr, "%s", str);
 	}
 
 	hfp->result_pending = true;
 
+	len = ringbuf_drain(hfp->read_buf, count + 1);
+
 	if (hfp->command_callback)
-		hfp->command_callback(str, hfp->command_data);
+		hfp->command_callback(ptr, hfp->command_data);
 	else
 		hfp_gw_send_result(hfp, HFP_RESULT_ERROR);
 
-	len = ringbuf_drain(hfp->read_buf, count + 1);
-
-	if (str == ptr)
-		free(ptr);
+	free(ptr);
 }
 
 static void read_watch_destroy(void *user_data)
@@ -341,6 +339,8 @@ bool hfp_gw_send_result(struct hfp_gw *hfp, enum hfp_result result)
 
 	hfp->result_pending = false;
 
+	process_input(hfp);
+
 	return true;
 }
 
@@ -356,6 +356,8 @@ bool hfp_gw_send_error(struct hfp_gw *hfp, enum hfp_error error)
 
 	hfp->result_pending = false;
 
+	process_input(hfp);
+
 	return true;
 }
 
-- 
1.8.3.1

[PATCHv2 2/8] shared/hfp: Add prefix handlers functionality

[Marcin Kraglak]

Add two functions: hfp_gw_set_prefix_handler() and
hfp_gw_remove_prefix_handler(). It will allow user to register for
specific command. Current implementation just adds or removes data
from hfp_gw structure.
---
 Makefile.tools   |  1 +
 src/shared/hfp.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/hfp.h | 17 ++++++++++++
 3 files changed, 102 insertions(+)

diff --git a/Makefile.tools b/Makefile.tools
index 9f7ba9f..31e1093 100644
--- a/Makefile.tools
+++ b/Makefile.tools
@@ -63,6 +63,7 @@ emulator_hfp_SOURCES = emulator/hfp.c \
 				monitor/mainloop.h monitor/mainloop.c \
 				src/shared/io.h src/shared/io-mainloop.c \
 				src/shared/util.h src/shared/util.c \
+				src/shared/queue.h src/shared/queue.c \
 				src/shared/ringbuf.h src/shared/ringbuf.c \
 				src/shared/hfp.h src/shared/hfp.c
 
diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 0681b19..e164dd6 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -32,6 +32,7 @@
 
 #include "src/shared/util.h"
 #include "src/shared/ringbuf.h"
+#include "src/shared/queue.h"
 #include "src/shared/io.h"
 #include "src/shared/hfp.h"
 
@@ -42,6 +43,7 @@ struct hfp_gw {
 	struct io *io;
 	struct ringbuf *read_buf;
 	struct ringbuf *write_buf;
+	struct queue *prefix_handlers;
 	bool writer_active;
 	bool permissive_syntax;
 	bool result_pending;
@@ -60,6 +62,37 @@ struct hfp_gw {
 	bool destroyed;
 };
 
+struct prefix_handler_data {
+	char *prefix;
+	void *user_data;
+	hfp_destroy_func_t destroy;
+	hfp_result_func_t callback;
+};
+
+static void destroy_prefix_handler_data(void *data)
+{
+	struct prefix_handler_data *handler = data;
+
+	if (handler->destroy)
+		handler->destroy(handler->user_data);
+
+	free(handler);
+}
+
+static bool match_handler_prefix(const void *a, const void *b)
+{
+	const struct prefix_handler_data *handler = a;
+	const char *prefix = b;
+
+	if (strlen(handler->prefix) != strlen(prefix))
+		return false;
+
+	if (memcmp(handler->prefix, prefix, strlen(prefix)))
+		return false;
+
+	return true;
+}
+
 static void write_watch_destroy(void *user_data)
 {
 	struct hfp_gw *hfp = user_data;
@@ -194,8 +227,19 @@ struct hfp_gw *hfp_gw_new(int fd)
 		return NULL;
 	}
 
+	hfp->prefix_handlers = queue_new();
+	if (!hfp->prefix_handlers) {
+		io_destroy(hfp->io);
+		ringbuf_free(hfp->write_buf);
+		ringbuf_free(hfp->read_buf);
+		free(hfp);
+		return NULL;
+	}
+
 	if (!io_set_read_handler(hfp->io, can_read_data,
 					hfp, read_watch_destroy)) {
+		queue_destroy(hfp->prefix_handlers,
+						destroy_prefix_handler_data);
 		io_destroy(hfp->io);
 		ringbuf_free(hfp->write_buf);
 		ringbuf_free(hfp->read_buf);
@@ -248,6 +292,9 @@ void hfp_gw_unref(struct hfp_gw *hfp)
 	ringbuf_free(hfp->write_buf);
 	hfp->write_buf = NULL;
 
+	queue_destroy(hfp->prefix_handlers, destroy_prefix_handler_data);
+	hfp->prefix_handlers = NULL;
+
 	if (!hfp->in_disconnect) {
 		free(hfp);
 		return;
@@ -407,6 +454,43 @@ bool hfp_gw_set_command_handler(struct hfp_gw *hfp,
 	return true;
 }
 
+bool hfp_gw_set_prefix_handler(struct hfp_gw *hfp, hfp_result_func_t callback,
+						char *prefix, void *user_data,
+						hfp_destroy_func_t destroy)
+{
+	struct prefix_handler_data *handler;
+
+	handler = queue_find(hfp->prefix_handlers, match_handler_prefix,
+									prefix);
+	if (handler)
+		return false;
+
+	handler = new0(struct prefix_handler_data, 1);
+	if (!handler)
+		return false;
+
+	handler->callback = callback;
+	handler->prefix = prefix;
+	handler->destroy = destroy;
+	handler->user_data = user_data;
+
+	return queue_push_tail(hfp->prefix_handlers, handler);
+}
+
+bool hfp_gw_remove_prefix_handler(struct hfp_gw *hfp, char *prefix)
+{
+	struct prefix_handler_data *handler;
+
+	handler = queue_remove_if(hfp->prefix_handlers, match_handler_prefix,
+									prefix);
+	if (!handler)
+		return false;
+
+	destroy_prefix_handler_data(handler);
+
+	return true;
+}
+
 static void disconnect_watch_destroy(void *user_data)
 {
 	struct hfp_gw *hfp = user_data;
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index b0bd934..306824d 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -60,6 +60,18 @@ enum hfp_error {
 	HFP_ERROR_NETWORK_NOT_ALLOWED		= 32,
 };
 
+enum hfp_cmd_type {
+	HFP_AT_READ,
+	HFP_AT_SET,
+	HFP_AT_TEST,
+	HFP_AT_COMMAND
+};
+
+struct hfp_gw_result;
+
+typedef void (*hfp_result_func_t)(struct hfp_gw_result *result,
+				enum hfp_cmd_type type, void *user_data);
+
 typedef void (*hfp_destroy_func_t)(void *user_data);
 typedef void (*hfp_debug_func_t)(const char *str, void *user_data);
 
@@ -94,3 +106,8 @@ bool hfp_gw_set_disconnect_handler(struct hfp_gw *hfp,
 					hfp_destroy_func_t destroy);
 
 bool hfp_gw_disconnect(struct hfp_gw *hfp);
+
+bool hfp_gw_set_prefix_handler(struct hfp_gw *hfp, hfp_result_func_t callback,
+						char *prefix, void *user_data,
+						hfp_destroy_func_t destroy);
+bool hfp_gw_remove_prefix_handler(struct hfp_gw *hfp, char *prefix);
-- 
1.8.3.1

[PATCHv2 3/8] shared/hfp: Add initial implementiation of processing commands

[Marcin Kraglak]

---
 src/shared/hfp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index e164dd6..cf54a8f 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -69,6 +69,11 @@ struct prefix_handler_data {
 	hfp_result_func_t callback;
 };
 
+struct hfp_gw_result {
+	const char *data;
+	int offset;
+};
+
 static void destroy_prefix_handler_data(void *data)
 {
 	struct prefix_handler_data *handler = data;
@@ -130,6 +135,46 @@ static void wakeup_writer(struct hfp_gw *hfp)
 	hfp->writer_active = true;
 }
 
+static bool process_basic(struct hfp_gw *hfp, struct hfp_gw_result *result)
+{
+	return false;
+}
+
+static bool process_extended(struct hfp_gw *hfp, struct hfp_gw_result *result)
+{
+	return false;
+}
+
+static void skip_whitespace(struct hfp_gw_result *result)
+{
+	while (result->data[result->offset] == ' ')
+		result->offset++;
+}
+
+static bool call_prefix_handler(struct hfp_gw *hfp, const char *data)
+{
+	struct hfp_gw_result result;
+
+	result.offset = 0;
+	result.data = data;
+
+	skip_whitespace(&result);
+
+	if (strlen(data + result.offset) < 3)
+		return false;
+
+	if (strncmp(data + result.offset, "AT", 2))
+		if (strncmp(data + result.offset, "at", 2))
+			return false;
+
+	result.offset += 2;
+
+	if (data[result.offset] == '+')
+		return process_extended(hfp, &result);
+	else
+		return process_basic(hfp, &result);
+}
+
 static void process_input(struct hfp_gw *hfp)
 {
 	char *str, *ptr;
@@ -163,10 +208,12 @@ static void process_input(struct hfp_gw *hfp)
 
 	len = ringbuf_drain(hfp->read_buf, count + 1);
 
-	if (hfp->command_callback)
-		hfp->command_callback(ptr, hfp->command_data);
-	else
-		hfp_gw_send_result(hfp, HFP_RESULT_ERROR);
+	if (!call_prefix_handler(hfp, ptr)) {
+		if (hfp->command_callback)
+			hfp->command_callback(ptr, hfp->command_data);
+		else
+			hfp_gw_send_result(hfp, HFP_RESULT_ERROR);
+	}
 
 	free(ptr);
 }
-- 
1.8.3.1

[PATCHv2 4/8] shared/hfp: Add implementation of precessing commands

[Marcin Kraglak]

It will process both extended and basic commands.
---
 src/shared/hfp.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 94 insertions(+), 2 deletions(-)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index cf54a8f..6a39a36 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -29,6 +29,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdarg.h>
+#include <ctype.h>
 
 #include "src/shared/util.h"
 #include "src/shared/ringbuf.h"
@@ -135,14 +136,105 @@ static void wakeup_writer(struct hfp_gw *hfp)
 	hfp->writer_active = true;
 }
 
+static enum hfp_cmd_type get_cmd_type(struct hfp_gw_result *result)
+{
+	if (result->data[result->offset] == '=') {
+		result->offset++;
+		if (result->data[result->offset] == '?') {
+			result->offset++;
+			return HFP_AT_TEST;
+		} else {
+			return HFP_AT_SET;
+		}
+	} else if (result->data[result->offset] == '?') {
+		result->offset++;
+		return HFP_AT_READ;
+	} else {
+		return HFP_AT_COMMAND;
+	}
+}
+
 static bool process_basic(struct hfp_gw *hfp, struct hfp_gw_result *result)
 {
-	return false;
+	const char *prefix = result->data + result->offset;
+	struct prefix_handler_data *handler;
+	enum hfp_cmd_type type;
+	char lookup_prefix[4];
+	uint8_t pref_len = 0;
+	int i;
+
+	/* Check if first sign is character */
+	if (isalpha(prefix[pref_len])) {
+		/* Handle S-parameter prefix */
+		if (toupper(prefix[pref_len]) == 'S') {
+			do {
+				pref_len++;
+			} while (isdigit(prefix[pref_len]));
+			/*S-parameter must be followed with number*/
+			if (pref_len == 1)
+				pref_len--;
+		} else {
+			/* It's just one-character prefix */
+			pref_len++;
+		}
+	}
+
+	if (pref_len == 0 || pref_len > sizeof(lookup_prefix))
+		return false;
+
+	for (i = 0; i < pref_len; i++)
+		lookup_prefix[i] = toupper(prefix[i]);
+
+	result->offset += pref_len;
+	lookup_prefix[pref_len] = '\0';
+
+	if (lookup_prefix[0] == 'D')
+		type = HFP_AT_SET;
+	else
+		type = get_cmd_type(result);
+
+	handler = queue_find(hfp->prefix_handlers, match_handler_prefix,
+								lookup_prefix);
+	if (!handler)
+		return false;
+
+	handler->callback(result, type, handler->user_data);
+
+	return true;
 }
 
 static bool process_extended(struct hfp_gw *hfp, struct hfp_gw_result *result)
 {
-	return false;
+	const char *prefix = result->data + result->offset;
+	const char *separators = ";?=\0";
+	struct prefix_handler_data *handler;
+	enum hfp_cmd_type type;
+	char lookup_prefix[18];
+	uint8_t pref_len;
+	int i;
+
+	/* Lookup for first separator */
+	pref_len = strcspn(prefix, separators);
+
+	if (pref_len > 17 || pref_len < 2)
+		return false;
+
+	for (i = 0; i < pref_len; i++)
+		lookup_prefix[i] = toupper(prefix[i]);
+
+	result->offset += pref_len;
+	lookup_prefix[pref_len] = '\0';
+
+	type = get_cmd_type(result);
+
+	handler = queue_find(hfp->prefix_handlers, match_handler_prefix,
+								lookup_prefix);
+	if (!handler)
+		return false;
+
+	handler->callback(result, type, handler->user_data);
+
+	return true;
 }
 
 static void skip_whitespace(struct hfp_gw_result *result)
-- 
1.8.3.1

[PATCHv2 5/8] shared/hfp: Add get_number implementation

[Marcin Kraglak]

User can call this function with hfp_gw_result, which will
be passed to prefix handler.
---
 src/shared/hfp.c | 33 +++++++++++++++++++++++++++++++++
 src/shared/hfp.h |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 6a39a36..f4cf3a4 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -237,12 +237,45 @@ static bool process_extended(struct hfp_gw *hfp, struct hfp_gw_result *result)
 	return true;
 }
 
+static void next_field(struct hfp_gw_result *result)
+{
+	if (result->data[result->offset] == ',')
+		result->offset++;
+}
+
 static void skip_whitespace(struct hfp_gw_result *result)
 {
 	while (result->data[result->offset] == ' ')
 		result->offset++;
 }
 
+bool hfp_gw_result_get_number(struct hfp_gw_result *result, int *val)
+{
+	int temp = 0;
+	int i;
+
+	skip_whitespace(result);
+
+	i = result->offset;
+
+	while (result->data[i] >= '0' && result->data[i] <= '9') {
+		temp *= 10;
+		temp += result->data[i++] - '0';
+	}
+
+	if (i == result->offset)
+		return false;
+
+	if (val)
+		*val = temp;
+	result->offset = i;
+
+	skip_whitespace(result);
+	next_field(result);
+
+	return true;
+}
+
 static bool call_prefix_handler(struct hfp_gw *hfp, const char *data)
 {
 	struct hfp_gw_result result;
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index 306824d..7642b7c 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -111,3 +111,5 @@ bool hfp_gw_set_prefix_handler(struct hfp_gw *hfp, hfp_result_func_t callback,
 						char *prefix, void *user_data,
 						hfp_destroy_func_t destroy);
 bool hfp_gw_remove_prefix_handler(struct hfp_gw *hfp, char *prefix);
+
+bool hfp_gw_result_get_number(struct hfp_gw_result *result, int *val);
-- 
1.8.3.1

[PATCHv2 6/8] shared/hfp: Add open/close container function

[Marcin Kraglak]

It will look for "(" or ")" parenthesis and skip it if found.
---
 src/shared/hfp.c | 26 ++++++++++++++++++++++++++
 src/shared/hfp.h |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index f4cf3a4..abecb31 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -276,6 +276,32 @@ bool hfp_gw_result_get_number(struct hfp_gw_result *result, int *val)
 	return true;
 }
 
+bool hfp_gw_result_open_container(struct hfp_gw_result *result)
+{
+	skip_whitespace(result);
+
+	/* The list shall be preceded by a left parenthesis "(") */
+	if (result->data[result->offset] != '(')
+		return false;
+
+	result->offset++;
+
+	return true;
+}
+
+bool hfp_gw_result_close_container(struct hfp_gw_result *result)
+{
+	skip_whitespace(result);
+
+	/* The list shall be followed by a right parenthesis (")" V250 5.7.3.1*/
+	if (result->data[result->offset] != ')')
+		return false;
+
+	result->offset++;
+
+	return true;
+}
+
 static bool call_prefix_handler(struct hfp_gw *hfp, const char *data)
 {
 	struct hfp_gw_result result;
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index 7642b7c..5ad214d 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -113,3 +113,5 @@ bool hfp_gw_set_prefix_handler(struct hfp_gw *hfp, hfp_result_func_t callback,
 bool hfp_gw_remove_prefix_handler(struct hfp_gw *hfp, char *prefix);
 
 bool hfp_gw_result_get_number(struct hfp_gw_result *result, int *val);
+bool hfp_gw_result_open_container(struct hfp_gw_result *result);
+bool hfp_gw_result_close_container(struct hfp_gw_result *result);
-- 
1.8.3.1

[PATCHv2 7/8] shared/hfp: Add function to get string

[Marcin Kraglak]

It will look for quoted sting and write it to given buffer.
---
 src/shared/hfp.c | 33 +++++++++++++++++++++++++++++++++
 src/shared/hfp.h |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index abecb31..5239fcd 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -302,6 +302,39 @@ bool hfp_gw_result_close_container(struct hfp_gw_result *result)
 	return true;
 }
 
+bool hfp_gw_result_get_string(struct hfp_gw_result *result, char *buf,
+								uint8_t len)
+{
+	int i = 0;
+	const char *data = result->data;
+
+	skip_whitespace(result);
+
+	if (data[result->offset] != '"')
+		return false;
+
+	result->offset++;
+
+	while (data[result->offset] != '\0' && data[result->offset] != '"') {
+		if (i < len)
+			buf[i++] = data[result->offset];
+		result->offset++;
+	}
+
+	if (i < len)
+		buf[i++] = '\0';
+
+	if (data[result->offset] == '"')
+		result->offset++;
+	else
+		return false;
+
+	skip_whitespace(result);
+	next_field(result);
+
+	return true;
+}
+
 static bool call_prefix_handler(struct hfp_gw *hfp, const char *data)
 {
 	struct hfp_gw_result result;
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index 5ad214d..4a69486 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -115,3 +115,5 @@ bool hfp_gw_remove_prefix_handler(struct hfp_gw *hfp, char *prefix);
 bool hfp_gw_result_get_number(struct hfp_gw_result *result, int *val);
 bool hfp_gw_result_open_container(struct hfp_gw_result *result);
 bool hfp_gw_result_close_container(struct hfp_gw_result *result);
+bool hfp_gw_result_get_string(struct hfp_gw_result *result, char *buf,
+								uint8_t len);
-- 
1.8.3.1

[PATCHv2 8/8] shared/hfp: Add function to get unquoted string

[Marcin Kraglak]

---
 src/shared/hfp.c | 28 ++++++++++++++++++++++++++++
 src/shared/hfp.h |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 5239fcd..2bed006 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -335,6 +335,34 @@ bool hfp_gw_result_get_string(struct hfp_gw_result *result, char *buf,
 	return true;
 }
 
+bool hfp_gw_result_get_unquoted_string(struct hfp_gw_result *result, char *buf,
+								uint8_t len)
+{
+	const char *data = result->data;
+	int i = 0;
+	char c;
+
+	skip_whitespace(result);
+
+	c = data[result->offset];
+	if (c == '"' || c == ')' || c == '(')
+		return false;
+
+	while (data[result->offset] != '\0' && data[result->offset] != ','
+					&& data[result->offset] != ')') {
+		if (i < len)
+			buf[i++] = data[result->offset];
+		result->offset++;
+	}
+
+	if (i < len)
+		buf[i++] = '\0';
+
+	next_field(result);
+
+	return true;
+}
+
 static bool call_prefix_handler(struct hfp_gw *hfp, const char *data)
 {
 	struct hfp_gw_result result;
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index 4a69486..5148253 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -117,3 +117,5 @@ bool hfp_gw_result_open_container(struct hfp_gw_result *result);
 bool hfp_gw_result_close_container(struct hfp_gw_result *result);
 bool hfp_gw_result_get_string(struct hfp_gw_result *result, char *buf,
 								uint8_t len);
+bool hfp_gw_result_get_unquoted_string(struct hfp_gw_result *result, char *buf,
+								uint8_t len);
-- 
1.8.3.1

Re: [PATCHv2 1/8] shared/hfp: Fix parsing line with few commands

[Marcel Holtmann]

Hi Marcin,

> If hfp_gw received line with few commands, it called command
> handler one time, for first command. Next comamnd was processed
> only if next line was received.
> Now, after every response from gw, we call proces_input to be
> sure that all data has been be processed.
> It will process next command only if response for previous was sent.
> ---
> src/shared/hfp.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
> index 854cf46..0681b19 100644
> --- a/src/shared/hfp.c
> +++ b/src/shared/hfp.c
> @@ -121,23 +121,21 @@ static void process_input(struct hfp_gw *hfp)
> 
> 		*ptr = '\0';
> 		count = asprintf(&ptr, "%s%s", str, str2);
> -		str = ptr;
> 	} else {
> -		count = ptr - str;
> 		*ptr = '\0';
> +		count = asprintf(&ptr, "%s", str);
> 	}

the whole point here is to not allocate a string if the command is not wrapped in the ring buffer. No idea what you are fixing here. And it is also not described in the commit message.

> 	hfp->result_pending = true;
> 
> +	len = ringbuf_drain(hfp->read_buf, count + 1);
> +
> 	if (hfp->command_callback)
> -		hfp->command_callback(str, hfp->command_data);
> +		hfp->command_callback(ptr, hfp->command_data);
> 	else
> 		hfp_gw_send_result(hfp, HFP_RESULT_ERROR);
> 
> -	len = ringbuf_drain(hfp->read_buf, count + 1);
> -
> -	if (str == ptr)
> -		free(ptr);
> +	free(ptr);
> }
> 
> static void read_watch_destroy(void *user_data)
> @@ -341,6 +339,8 @@ bool hfp_gw_send_result(struct hfp_gw *hfp, enum hfp_result result)
> 
> 	hfp->result_pending = false;
> 
> +	process_input(hfp);
> +
> 	return true;
> }
> 
> @@ -356,6 +356,8 @@ bool hfp_gw_send_error(struct hfp_gw *hfp, enum hfp_error error)
> 
> 	hfp->result_pending = false;
> 
> +	process_input(hfp);
> +
> 	return true;
> }

We can certainly do this, but normally AT commands work that you get one, send a response and then get the next one. Except for unsolicited notification, but in HFP gateway case, we are the ones sending these and not the headset.

What is this all fixing?

Regards

Marcel

Re: [PATCHv2 3/8] shared/hfp: Add initial implementiation of processing commands

[Marcel Holtmann]

Hi Marcin,

> ---
> src/shared/hfp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
> index e164dd6..cf54a8f 100644
> --- a/src/shared/hfp.c
> +++ b/src/shared/hfp.c
> @@ -69,6 +69,11 @@ struct prefix_handler_data {
> 	hfp_result_func_t callback;
> };
> 
> +struct hfp_gw_result {
> +	const char *data;
> +	int offset;
> +};
> +
> static void destroy_prefix_handler_data(void *data)
> {
> 	struct prefix_handler_data *handler = data;
> @@ -130,6 +135,46 @@ static void wakeup_writer(struct hfp_gw *hfp)
> 	hfp->writer_active = true;
> }
> 
> +static bool process_basic(struct hfp_gw *hfp, struct hfp_gw_result *result)
> +{
> +	return false;
> +}
> +
> +static bool process_extended(struct hfp_gw *hfp, struct hfp_gw_result *result)
> +{
> +	return false;
> +}
> +
> +static void skip_whitespace(struct hfp_gw_result *result)
> +{
> +	while (result->data[result->offset] == ' ')
> +		result->offset++;
> +}
> +
> +static bool call_prefix_handler(struct hfp_gw *hfp, const char *data)
> +{
> +	struct hfp_gw_result result;
> +
> +	result.offset = 0;
> +	result.data = data;
> +
> +	skip_whitespace(&result);

I would just do the skip leading whitespace here without bothering putting it into a separate function. There is really no point.

> +
> +	if (strlen(data + result.offset) < 3)
> +		return false;
> +
> +	if (strncmp(data + result.offset, "AT", 2))
> +		if (strncmp(data + result.offset, "at", 2))
> +			return false;
> +
> +	result.offset += 2;
> +
> +	if (data[result.offset] == '+')
> +		return process_extended(hfp, &result);
> +	else
> +		return process_basic(hfp, &result);

Please do not do this. I mentioned this before, this basic vs extended is pointless.

Command matching should be either based on commands like “D” or “+BRSF” and not bother trying to treat the + any special. See how src/emulator.c in oFono registers all the handlers.

Regards

Marcel

Re: [PATCHv2 2/8] shared/hfp: Add prefix handlers functionality

[Marcel Holtmann]

Hi Marcin,

> Add two functions: hfp_gw_set_prefix_handler() and
> hfp_gw_remove_prefix_handler(). It will allow user to register for
> specific command. Current implementation just adds or removes data
> from hfp_gw structure.
> ---
> Makefile.tools   |  1 +
> src/shared/hfp.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> src/shared/hfp.h | 17 ++++++++++++
> 3 files changed, 102 insertions(+)
> 
> diff --git a/Makefile.tools b/Makefile.tools
> index 9f7ba9f..31e1093 100644
> --- a/Makefile.tools
> +++ b/Makefile.tools
> @@ -63,6 +63,7 @@ emulator_hfp_SOURCES = emulator/hfp.c \
> 				monitor/mainloop.h monitor/mainloop.c \
> 				src/shared/io.h src/shared/io-mainloop.c \
> 				src/shared/util.h src/shared/util.c \
> +				src/shared/queue.h src/shared/queue.c \
> 				src/shared/ringbuf.h src/shared/ringbuf.c \
> 				src/shared/hfp.h src/shared/hfp.c
> 
> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
> index 0681b19..e164dd6 100644
> --- a/src/shared/hfp.c
> +++ b/src/shared/hfp.c
> @@ -32,6 +32,7 @@
> 
> #include "src/shared/util.h"
> #include "src/shared/ringbuf.h"
> +#include "src/shared/queue.h"
> #include "src/shared/io.h"
> #include "src/shared/hfp.h"
> 
> @@ -42,6 +43,7 @@ struct hfp_gw {
> 	struct io *io;
> 	struct ringbuf *read_buf;
> 	struct ringbuf *write_buf;
> +	struct queue *prefix_handlers;
> 	bool writer_active;
> 	bool permissive_syntax;
> 	bool result_pending;
> @@ -60,6 +62,37 @@ struct hfp_gw {
> 	bool destroyed;
> };
> 
> +struct prefix_handler_data {
> +	char *prefix;
> +	void *user_data;
> +	hfp_destroy_func_t destroy;
> +	hfp_result_func_t callback;
> +};
> +
> +static void destroy_prefix_handler_data(void *data)
> +{
> +	struct prefix_handler_data *handler = data;
> +
> +	if (handler->destroy)
> +		handler->destroy(handler->user_data);
> +
> +	free(handler);
> +}
> +
> +static bool match_handler_prefix(const void *a, const void *b)
> +{
> +	const struct prefix_handler_data *handler = a;
> +	const char *prefix = b;
> +
> +	if (strlen(handler->prefix) != strlen(prefix))
> +		return false;
> +
> +	if (memcmp(handler->prefix, prefix, strlen(prefix)))
> +		return false;
> +
> +	return true;
> +}
> +
> static void write_watch_destroy(void *user_data)
> {
> 	struct hfp_gw *hfp = user_data;
> @@ -194,8 +227,19 @@ struct hfp_gw *hfp_gw_new(int fd)
> 		return NULL;
> 	}
> 
> +	hfp->prefix_handlers = queue_new();
> +	if (!hfp->prefix_handlers) {
> +		io_destroy(hfp->io);
> +		ringbuf_free(hfp->write_buf);
> +		ringbuf_free(hfp->read_buf);
> +		free(hfp);
> +		return NULL;
> +	}
> +
> 	if (!io_set_read_handler(hfp->io, can_read_data,
> 					hfp, read_watch_destroy)) {
> +		queue_destroy(hfp->prefix_handlers,
> +						destroy_prefix_handler_data);
> 		io_destroy(hfp->io);
> 		ringbuf_free(hfp->write_buf);
> 		ringbuf_free(hfp->read_buf);
> @@ -248,6 +292,9 @@ void hfp_gw_unref(struct hfp_gw *hfp)
> 	ringbuf_free(hfp->write_buf);
> 	hfp->write_buf = NULL;
> 
> +	queue_destroy(hfp->prefix_handlers, destroy_prefix_handler_data);
> +	hfp->prefix_handlers = NULL;
> +
> 	if (!hfp->in_disconnect) {
> 		free(hfp);
> 		return;
> @@ -407,6 +454,43 @@ bool hfp_gw_set_command_handler(struct hfp_gw *hfp,
> 	return true;
> }
> 
> +bool hfp_gw_set_prefix_handler(struct hfp_gw *hfp, hfp_result_func_t callback,
> +						char *prefix, void *user_data,
> +						hfp_destroy_func_t destroy)
> +{

it is either add and remove or register and unregister, but never set and remove. If you set something, the next call to set would overwrite it.

Regards

Marcel

Re: [PATCHv2 1/8] shared/hfp: Fix parsing line with few commands

[Marcin Kraglak]

Hi Marcel,

On 28 February 2014 08:00, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Marcin,
>
>> If hfp_gw received line with few commands, it called command
>> handler one time, for first command. Next comamnd was processed
>> only if next line was received.
>> Now, after every response from gw, we call proces_input to be
>> sure that all data has been be processed.
>> It will process next command only if response for previous was sent.
>> ---
>> src/shared/hfp.c | 16 +++++++++-------
>> 1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index 854cf46..0681b19 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -121,23 +121,21 @@ static void process_input(struct hfp_gw *hfp)
>>
>>               *ptr = '\0';
>>               count = asprintf(&ptr, "%s%s", str, str2);
>> -             str = ptr;
>>       } else {
>> -             count = ptr - str;
>>               *ptr = '\0';
>> +             count = asprintf(&ptr, "%s", str);
>>       }
>
> the whole point here is to not allocate a string if the command is not wrapped in the ring buffer. No idea what you are fixing here. And it is also not described in the commit message.
>
>>       hfp->result_pending = true;
>>
>> +     len = ringbuf_drain(hfp->read_buf, count + 1);
>> +
>>       if (hfp->command_callback)
>> -             hfp->command_callback(str, hfp->command_data);
>> +             hfp->command_callback(ptr, hfp->command_data);
>>       else
>>               hfp_gw_send_result(hfp, HFP_RESULT_ERROR);
>>
>> -     len = ringbuf_drain(hfp->read_buf, count + 1);
>> -
>> -     if (str == ptr)
>> -             free(ptr);
>> +     free(ptr);
>> }
>>
>> static void read_watch_destroy(void *user_data)
>> @@ -341,6 +339,8 @@ bool hfp_gw_send_result(struct hfp_gw *hfp, enum hfp_result result)
>>
>>       hfp->result_pending = false;
>>
>> +     process_input(hfp);
>> +
>>       return true;
>> }
>>
>> @@ -356,6 +356,8 @@ bool hfp_gw_send_error(struct hfp_gw *hfp, enum hfp_error error)
>>
>>       hfp->result_pending = false;
>>
>> +     process_input(hfp);
>> +
>>       return true;
>> }
>
> We can certainly do this, but normally AT commands work that you get one, send a response and then get the next one. Except for unsolicited notification, but in HFP gateway case, we are the ones sending these and not the headset.
>
> What is this all fixing?
>
> Regards
>
> Marcel
>
If we can receive one command in time, lets just ignore this patch

BR
Marcin

Re: [PATCHv2 4/8] shared/hfp: Add implementation of precessing commands

[Marcel Holtmann]

Hi Marcin,

> It will process both extended and basic commands.
> ---
> src/shared/hfp.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 94 insertions(+), 2 deletions(-)
> 
> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
> index cf54a8f..6a39a36 100644
> --- a/src/shared/hfp.c
> +++ b/src/shared/hfp.c
> @@ -29,6 +29,7 @@
> #include <unistd.h>
> #include <string.h>
> #include <stdarg.h>
> +#include <ctype.h>
> 
> #include "src/shared/util.h"
> #include "src/shared/ringbuf.h"
> @@ -135,14 +136,105 @@ static void wakeup_writer(struct hfp_gw *hfp)
> 	hfp->writer_active = true;
> }
> 
> +static enum hfp_cmd_type get_cmd_type(struct hfp_gw_result *result)
> +{
> +	if (result->data[result->offset] == '=') {
> +		result->offset++;
> +		if (result->data[result->offset] == '?') {
> +			result->offset++;
> +			return HFP_AT_TEST;
> +		} else {
> +			return HFP_AT_SET;
> +		}

can we please keep the kernel coding style out of user space. And I hate it when both if parts use return.

	if (blah) {
		xxxx
		return x;
	}

	return y.

> +	} else if (result->data[result->offset] == '?') {

And this else if is pointless as well. You are leaving the function in the first if part no matter what.

> +		result->offset++;
> +		return HFP_AT_READ;
> +	} else {
> +		return HFP_AT_COMMAND;
> +	}

Same here.

> +}
> +
> static bool process_basic(struct hfp_gw *hfp, struct hfp_gw_result *result)
> {
> -	return false;
> +	const char *prefix = result->data + result->offset;
> +	struct prefix_handler_data *handler;
> +	enum hfp_cmd_type type;
> +	char lookup_prefix[4];
> +	uint8_t pref_len = 0;
> +	int i;
> +
> +	/* Check if first sign is character */
> +	if (isalpha(prefix[pref_len])) {
> +		/* Handle S-parameter prefix */
> +		if (toupper(prefix[pref_len]) == 'S') {
> +			do {
> +				pref_len++;
> +			} while (isdigit(prefix[pref_len]));
> +			/*S-parameter must be followed with number*/
> +			if (pref_len == 1)
> +				pref_len—;

Where is the S parameter stuff coming from? I did ask this before and never got an answer to it.

Regards

Marcel

Re: [PATCHv2 3/8] shared/hfp: Add initial implementiation of processing commands

[Marcin Kraglak]

Hi Marcel,

On 28 February 2014 08:05, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Marcin,
>
>> ---
>> src/shared/hfp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 51 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index e164dd6..cf54a8f 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -69,6 +69,11 @@ struct prefix_handler_data {
>>       hfp_result_func_t callback;
>> };
>>
>> +struct hfp_gw_result {
>> +     const char *data;
>> +     int offset;
>> +};
>> +
>> static void destroy_prefix_handler_data(void *data)
>> {
>>       struct prefix_handler_data *handler = data;
>> @@ -130,6 +135,46 @@ static void wakeup_writer(struct hfp_gw *hfp)
>>       hfp->writer_active = true;
>> }
>>
>> +static bool process_basic(struct hfp_gw *hfp, struct hfp_gw_result *result)
>> +{
>> +     return false;
>> +}
>> +
>> +static bool process_extended(struct hfp_gw *hfp, struct hfp_gw_result *result)
>> +{
>> +     return false;
>> +}
>> +
>> +static void skip_whitespace(struct hfp_gw_result *result)
>> +{
>> +     while (result->data[result->offset] == ' ')
>> +             result->offset++;
>> +}
>> +
>> +static bool call_prefix_handler(struct hfp_gw *hfp, const char *data)
>> +{
>> +     struct hfp_gw_result result;
>> +
>> +     result.offset = 0;
>> +     result.data = data;
>> +
>> +     skip_whitespace(&result);
>
> I would just do the skip leading whitespace here without bothering putting it into a separate function. There is really no point.

I use this function later, in getters, therefore I introduced it here
>
>> +
>> +     if (strlen(data + result.offset) < 3)
>> +             return false;
>> +
>> +     if (strncmp(data + result.offset, "AT", 2))
>> +             if (strncmp(data + result.offset, "at", 2))
>> +                     return false;
>> +
>> +     result.offset += 2;
>> +
>> +     if (data[result.offset] == '+')
>> +             return process_extended(hfp, &result);
>> +     else
>> +             return process_basic(hfp, &result);
>
> Please do not do this. I mentioned this before, this basic vs extended is pointless.
>
> Command matching should be either based on commands like "D" or "+BRSF" and not bother trying to treat the + any special. See how src/emulator.c in oFono registers all the handlers.

Ok, I'll fix it
>
> Regards
>
> Marcel
>
BR
Marcin

Re: [PATCHv2 4/8] shared/hfp: Add implementation of precessing commands

[Marcin Kraglak]

Hi Marcel,

On 28 February 2014 08:13, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Marcin,
>
>> It will process both extended and basic commands.
>> ---
>> src/shared/hfp.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 94 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index cf54a8f..6a39a36 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -29,6 +29,7 @@
>> #include <unistd.h>
>> #include <string.h>
>> #include <stdarg.h>
>> +#include <ctype.h>
>>
>> #include "src/shared/util.h"
>> #include "src/shared/ringbuf.h"
>> @@ -135,14 +136,105 @@ static void wakeup_writer(struct hfp_gw *hfp)
>>       hfp->writer_active = true;
>> }
>>
>> +static enum hfp_cmd_type get_cmd_type(struct hfp_gw_result *result)
>> +{
>> +     if (result->data[result->offset] == '=') {
>> +             result->offset++;
>> +             if (result->data[result->offset] == '?') {
>> +                     result->offset++;
>> +                     return HFP_AT_TEST;
>> +             } else {
>> +                     return HFP_AT_SET;
>> +             }
>
> can we please keep the kernel coding style out of user space. And I hate it when both if parts use return.
>
>         if (blah) {
>                 xxxx
>                 return x;
>         }
>
>         return y.
>
>> +     } else if (result->data[result->offset] == '?') {
>
> And this else if is pointless as well. You are leaving the function in the first if part no matter what.
>
>> +             result->offset++;
>> +             return HFP_AT_READ;
>> +     } else {
>> +             return HFP_AT_COMMAND;
>> +     }
>
> Same here.
>
>> +}
>> +
>> static bool process_basic(struct hfp_gw *hfp, struct hfp_gw_result *result)
>> {
>> -     return false;
>> +     const char *prefix = result->data + result->offset;
>> +     struct prefix_handler_data *handler;
>> +     enum hfp_cmd_type type;
>> +     char lookup_prefix[4];
>> +     uint8_t pref_len = 0;
>> +     int i;
>> +
>> +     /* Check if first sign is character */
>> +     if (isalpha(prefix[pref_len])) {
>> +             /* Handle S-parameter prefix */
>> +             if (toupper(prefix[pref_len]) == 'S') {
>> +                     do {
>> +                             pref_len++;
>> +                     } while (isdigit(prefix[pref_len]));
>> +                     /*S-parameter must be followed with number*/
>> +                     if (pref_len == 1)
>> +                             pref_len--;
>
> Where is the S parameter stuff coming from? I did ask this before and never got an answer to it.

V.250 5.3.2 S-parameters. If it is not used in HFP I can remove it. It
was added just to be compatibile with V.250 spec.
>
> Regards
>
> Marcel
>
BR
Marcin

Re: [PATCHv2 2/8] shared/hfp: Add prefix handlers functionality

[Marcin Kraglak]

Hi Marcel,

On 28 February 2014 08:06, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Marcin,
>
>> Add two functions: hfp_gw_set_prefix_handler() and
>> hfp_gw_remove_prefix_handler(). It will allow user to register for
>> specific command. Current implementation just adds or removes data
>> from hfp_gw structure.
>> ---
>> Makefile.tools   |  1 +
>> src/shared/hfp.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> src/shared/hfp.h | 17 ++++++++++++
>> 3 files changed, 102 insertions(+)
>>
>> diff --git a/Makefile.tools b/Makefile.tools
>> index 9f7ba9f..31e1093 100644
>> --- a/Makefile.tools
>> +++ b/Makefile.tools
>> @@ -63,6 +63,7 @@ emulator_hfp_SOURCES = emulator/hfp.c \
>>                               monitor/mainloop.h monitor/mainloop.c \
>>                               src/shared/io.h src/shared/io-mainloop.c \
>>                               src/shared/util.h src/shared/util.c \
>> +                             src/shared/queue.h src/shared/queue.c \
>>                               src/shared/ringbuf.h src/shared/ringbuf.c \
>>                               src/shared/hfp.h src/shared/hfp.c
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index 0681b19..e164dd6 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -32,6 +32,7 @@
>>
>> #include "src/shared/util.h"
>> #include "src/shared/ringbuf.h"
>> +#include "src/shared/queue.h"
>> #include "src/shared/io.h"
>> #include "src/shared/hfp.h"
>>
>> @@ -42,6 +43,7 @@ struct hfp_gw {
>>       struct io *io;
>>       struct ringbuf *read_buf;
>>       struct ringbuf *write_buf;
>> +     struct queue *prefix_handlers;
>>       bool writer_active;
>>       bool permissive_syntax;
>>       bool result_pending;
>> @@ -60,6 +62,37 @@ struct hfp_gw {
>>       bool destroyed;
>> };
>>
>> +struct prefix_handler_data {
>> +     char *prefix;
>> +     void *user_data;
>> +     hfp_destroy_func_t destroy;
>> +     hfp_result_func_t callback;
>> +};
>> +
>> +static void destroy_prefix_handler_data(void *data)
>> +{
>> +     struct prefix_handler_data *handler = data;
>> +
>> +     if (handler->destroy)
>> +             handler->destroy(handler->user_data);
>> +
>> +     free(handler);
>> +}
>> +
>> +static bool match_handler_prefix(const void *a, const void *b)
>> +{
>> +     const struct prefix_handler_data *handler = a;
>> +     const char *prefix = b;
>> +
>> +     if (strlen(handler->prefix) != strlen(prefix))
>> +             return false;
>> +
>> +     if (memcmp(handler->prefix, prefix, strlen(prefix)))
>> +             return false;
>> +
>> +     return true;
>> +}
>> +
>> static void write_watch_destroy(void *user_data)
>> {
>>       struct hfp_gw *hfp = user_data;
>> @@ -194,8 +227,19 @@ struct hfp_gw *hfp_gw_new(int fd)
>>               return NULL;
>>       }
>>
>> +     hfp->prefix_handlers = queue_new();
>> +     if (!hfp->prefix_handlers) {
>> +             io_destroy(hfp->io);
>> +             ringbuf_free(hfp->write_buf);
>> +             ringbuf_free(hfp->read_buf);
>> +             free(hfp);
>> +             return NULL;
>> +     }
>> +
>>       if (!io_set_read_handler(hfp->io, can_read_data,
>>                                       hfp, read_watch_destroy)) {
>> +             queue_destroy(hfp->prefix_handlers,
>> +                                             destroy_prefix_handler_data);
>>               io_destroy(hfp->io);
>>               ringbuf_free(hfp->write_buf);
>>               ringbuf_free(hfp->read_buf);
>> @@ -248,6 +292,9 @@ void hfp_gw_unref(struct hfp_gw *hfp)
>>       ringbuf_free(hfp->write_buf);
>>       hfp->write_buf = NULL;
>>
>> +     queue_destroy(hfp->prefix_handlers, destroy_prefix_handler_data);
>> +     hfp->prefix_handlers = NULL;
>> +
>>       if (!hfp->in_disconnect) {
>>               free(hfp);
>>               return;
>> @@ -407,6 +454,43 @@ bool hfp_gw_set_command_handler(struct hfp_gw *hfp,
>>       return true;
>> }
>>
>> +bool hfp_gw_set_prefix_handler(struct hfp_gw *hfp, hfp_result_func_t callback,
>> +                                             char *prefix, void *user_data,
>> +                                             hfp_destroy_func_t destroy)
>> +{
>
> it is either add and remove or register and unregister, but never set and remove. If you set something, the next call to set would overwrite it.
>
> Regards
>
> Marcel
>


I'll change names

BR
Marcin

Re: [PATCHv2 3/8] shared/hfp: Add initial implementiation of processing commands

[Denis Kenzior]

Hi Marcel,

 >> +	if (data[result.offset] == '+')
>> +		return process_extended(hfp, &result);
>> +	else
>> +		return process_basic(hfp, &result);
>
> Please do not do this. I mentioned this before, this basic vs extended is pointless.
>
> Command matching should be either based on commands like “D” or “+BRSF” and not bother trying to treat the + any special. See how src/emulator.c in oFono registers all the handlers.
>

There is a difference in syntax between basic commands and extended 
commands.  HFP only uses two basic commands, namely ATD and ATA so they 
might be simply handled as a special case.  Others are implicit, e.g. 
ATE, ATV, etc.

Regards,
-Denis