[PATCH v4 00/11] shared/hfp: Add support for HFP HF

[PATCH v4 00/11] shared/hfp: Add support for HFP HF

[Lukasz Rymanowski]

Following patches extends hfp API with HFP HF functionality.
HFP HF parser has been added and unit test for it.

To consider: how strict we should be when it comes to parsing
AT responses. For example, at the moment, command +CCLC:<cr><lf>
will be recognized as +CCLC: eventhough correct response format
should be <cr><lf>+CCLC:<cr><lf>

Note: As discussed on IRC I did not try to generalize code.

v2:
* minor self review fixes
* response callback on send command, contains now result (OK/ERROR) and
data from unsolicited response if available.

v3:
* Fix some memory leaks found on self review

v4:
* Fallback to approach from v1 in context of response callback for AT command.
Bassically, if AT+X has +X and OK response, response callback contains only OK or
ERROR code (including CME which will be added in following patches). To get +X
response, user need to use hfp_hf_register() API. It is done mostly to keep hfp.c
simple. With this approach we do not have to cache all +X in hfp.c before calling
response callback.

Lukasz Rymanowski (11):
  shared/hfp: Add support for HFP HF
  shared/hfp: Add set_debug and close_on_unref API for HFP HF
  shared/hfp: Add set disconnect handler and disconnect API to HFP HF
  shared/hfp: Add register/unregister event for HFP HF
  shared/hfp: Add HFP HF parser
  shared/hfp: Add send AT command API for HFP HF
  unit/test-hfp: Provide test_handler function via struct data
  unit/test-hfp: Add init test for HFP HF
  unit/test-hfp: Add send command tests for HFP HF
  unit/test-hfp: Add tests for unsolicited results for HFP HF
  unit/test-hfp: Add some robustness tests for HFP HF

 src/shared/hfp.c | 624 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/hfp.h |  30 +++
 unit/test-hfp.c  | 285 +++++++++++++++++++++++--
 3 files changed, 920 insertions(+), 19 deletions(-)

-- 
1.8.4

[PATCH v4 01/11] shared/hfp: Add support for HFP HF

[Lukasz Rymanowski]

This patch add struct hfp_hf plus fuctions to create an instance ref and
unref. This code based on existing hfp_gw
---
 src/shared/hfp.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/hfp.h |  6 ++++
 2 files changed, 98 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index efc981f..dbd049a 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -62,6 +62,18 @@ struct hfp_gw {
 	bool destroyed;
 };
 
+struct hfp_hf {
+	int ref_count;
+	int fd;
+	bool close_on_unref;
+	struct io *io;
+	struct ringbuf *read_buf;
+	struct ringbuf *write_buf;
+
+	bool in_disconnect;
+	bool destroyed;
+};
+
 struct cmd_handler {
 	char *prefix;
 	void *user_data;
@@ -807,3 +819,83 @@ bool hfp_gw_disconnect(struct hfp_gw *hfp)
 
 	return io_shutdown(hfp->io);
 }
+
+struct hfp_hf *hfp_hf_new(int fd)
+{
+	struct hfp_hf *hfp;
+
+	if (fd < 0)
+		return NULL;
+
+	hfp = new0(struct hfp_hf, 1);
+	if (!hfp)
+		return NULL;
+
+	hfp->fd = fd;
+	hfp->close_on_unref = false;
+
+	hfp->read_buf = ringbuf_new(4096);
+	if (!hfp->read_buf) {
+		free(hfp);
+		return NULL;
+	}
+
+	hfp->write_buf = ringbuf_new(4096);
+	if (!hfp->write_buf) {
+		ringbuf_free(hfp->read_buf);
+		free(hfp);
+		return NULL;
+	}
+
+	hfp->io = io_new(fd);
+	if (!hfp->io) {
+		ringbuf_free(hfp->write_buf);
+		ringbuf_free(hfp->read_buf);
+		free(hfp);
+		return NULL;
+	}
+
+	return hfp_hf_ref(hfp);
+}
+
+struct hfp_hf *hfp_hf_ref(struct hfp_hf *hfp)
+{
+	if (!hfp)
+		return NULL;
+
+	__sync_fetch_and_add(&hfp->ref_count, 1);
+
+	return hfp;
+}
+
+void hfp_hf_unref(struct hfp_hf *hfp)
+{
+	if (!hfp)
+		return;
+
+	if (__sync_sub_and_fetch(&hfp->ref_count, 1))
+		return;
+
+	io_set_write_handler(hfp->io, NULL, NULL, NULL);
+	io_set_read_handler(hfp->io, NULL, NULL, NULL);
+	io_set_disconnect_handler(hfp->io, NULL, NULL, NULL);
+
+	io_destroy(hfp->io);
+	hfp->io = NULL;
+
+	if (hfp->close_on_unref)
+		close(hfp->fd);
+
+	ringbuf_free(hfp->read_buf);
+	hfp->read_buf = NULL;
+
+	ringbuf_free(hfp->write_buf);
+	hfp->write_buf = NULL;
+
+	if (!hfp->in_disconnect) {
+		free(hfp);
+		return;
+	}
+
+	hfp->destroyed = true;
+}
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index 743db65..50d9c4b 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -76,9 +76,11 @@ typedef void (*hfp_destroy_func_t)(void *user_data);
 typedef void (*hfp_debug_func_t)(const char *str, void *user_data);
 
 typedef void (*hfp_command_func_t)(const char *command, void *user_data);
+
 typedef void (*hfp_disconnect_func_t)(void *user_data);
 
 struct hfp_gw;
+struct hfp_hf;
 
 struct hfp_gw *hfp_gw_new(int fd);
 
@@ -124,3 +126,7 @@ bool hfp_gw_result_get_string(struct hfp_gw_result *result, char *buf,
 bool hfp_gw_result_get_unquoted_string(struct hfp_gw_result *result, char *buf,
 								uint8_t len);
 bool hfp_gw_result_has_next(struct hfp_gw_result *result);
+
+struct hfp_hf *hfp_hf_new(int fd);
+struct hfp_hf *hfp_hf_ref(struct hfp_hf *hfp);
+void hfp_hf_unref(struct hfp_hf *hfp);
-- 
1.8.4

[PATCH v4 02/11] shared/hfp: Add set_debug and close_on_unref API for HFP HF

[Lukasz Rymanowski]

---
 src/shared/hfp.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/hfp.h |  3 +++
 2 files changed, 60 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index dbd049a..ad2daa2 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -70,6 +70,10 @@ struct hfp_hf {
 	struct ringbuf *read_buf;
 	struct ringbuf *write_buf;
 
+	hfp_debug_func_t debug_callback;
+	hfp_destroy_func_t debug_destroy;
+	void *debug_data;
+
 	bool in_disconnect;
 	bool destroyed;
 };
@@ -886,6 +890,8 @@ void hfp_hf_unref(struct hfp_hf *hfp)
 	if (hfp->close_on_unref)
 		close(hfp->fd);
 
+	hfp_hf_set_debug(hfp, NULL, NULL, NULL);
+
 	ringbuf_free(hfp->read_buf);
 	hfp->read_buf = NULL;
 
@@ -899,3 +905,54 @@ void hfp_hf_unref(struct hfp_hf *hfp)
 
 	hfp->destroyed = true;
 }
+
+static void hf_read_tracing(const void *buf, size_t count,
+							void *user_data)
+{
+	struct hfp_hf *hfp = user_data;
+
+	util_hexdump('>', buf, count, hfp->debug_callback, hfp->debug_data);
+}
+
+static void hf_write_tracing(const void *buf, size_t count,
+							void *user_data)
+{
+	struct hfp_hf *hfp = user_data;
+
+	util_hexdump('<', buf, count, hfp->debug_callback, hfp->debug_data);
+}
+
+bool hfp_hf_set_debug(struct hfp_hf *hfp, hfp_debug_func_t callback,
+				void *user_data, hfp_destroy_func_t destroy)
+{
+	if (!hfp)
+		return false;
+
+	if (hfp->debug_destroy)
+		hfp->debug_destroy(hfp->debug_data);
+
+	hfp->debug_callback = callback;
+	hfp->debug_destroy = destroy;
+	hfp->debug_data = user_data;
+
+	if (hfp->debug_callback) {
+		ringbuf_set_input_tracing(hfp->read_buf, hf_read_tracing, hfp);
+		ringbuf_set_input_tracing(hfp->write_buf, hf_write_tracing,
+									hfp);
+	} else {
+		ringbuf_set_input_tracing(hfp->read_buf, NULL, NULL);
+		ringbuf_set_input_tracing(hfp->write_buf, NULL, NULL);
+	}
+
+	return true;
+}
+
+bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close)
+{
+	if (!hfp)
+		return false;
+
+	hfp->close_on_unref = do_close;
+
+	return true;
+}
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index 50d9c4b..ae67ac2 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -130,3 +130,6 @@ bool hfp_gw_result_has_next(struct hfp_gw_result *result);
 struct hfp_hf *hfp_hf_new(int fd);
 struct hfp_hf *hfp_hf_ref(struct hfp_hf *hfp);
 void hfp_hf_unref(struct hfp_hf *hfp);
+bool hfp_hf_set_debug(struct hfp_hf *hfp, hfp_debug_func_t callback,
+				void *user_data, hfp_destroy_func_t destroy);
+bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close);
-- 
1.8.4

[PATCH v4 03/11] shared/hfp: Add set disconnect handler and disconnect API to HFP HF

[Lukasz Rymanowski]

---
 src/shared/hfp.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/hfp.h |  5 +++++
 2 files changed, 68 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index ad2daa2..b7855ed 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -74,6 +74,10 @@ struct hfp_hf {
 	hfp_destroy_func_t debug_destroy;
 	void *debug_data;
 
+	hfp_disconnect_func_t disconnect_callback;
+	hfp_destroy_func_t disconnect_destroy;
+	void *disconnect_data;
+
 	bool in_disconnect;
 	bool destroyed;
 };
@@ -956,3 +960,62 @@ bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close)
 
 	return true;
 }
+
+static void hf_disconnect_watch_destroy(void *user_data)
+{
+	struct hfp_hf *hfp = user_data;
+
+	if (hfp->disconnect_destroy)
+		hfp->disconnect_destroy(hfp->disconnect_data);
+
+	if (hfp->destroyed)
+		free(hfp);
+}
+
+static bool hf_io_disconnected(struct io *io, void *user_data)
+{
+	struct hfp_hf *hfp = user_data;
+
+	hfp->in_disconnect = true;
+
+	if (hfp->disconnect_callback)
+		hfp->disconnect_callback(hfp->disconnect_data);
+
+	hfp->in_disconnect = false;
+
+	return false;
+}
+
+bool hfp_hf_set_disconnect_handler(struct hfp_hf *hfp,
+						hfp_disconnect_func_t callback,
+						void *user_data,
+						hfp_destroy_func_t destroy)
+{
+	if (!hfp)
+		return false;
+
+	if (hfp->disconnect_destroy)
+		hfp->disconnect_destroy(hfp->disconnect_data);
+
+	if (!io_set_disconnect_handler(hfp->io, hf_io_disconnected, hfp,
+						hf_disconnect_watch_destroy)) {
+		hfp->disconnect_callback = NULL;
+		hfp->disconnect_destroy = NULL;
+		hfp->disconnect_data = NULL;
+		return false;
+	}
+
+	hfp->disconnect_callback = callback;
+	hfp->disconnect_destroy = destroy;
+	hfp->disconnect_data = user_data;
+
+	return true;
+}
+
+bool hfp_hf_disconnect(struct hfp_hf *hfp)
+{
+	if (!hfp)
+		return false;
+
+	return io_shutdown(hfp->io);
+}
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index ae67ac2..a9a169b 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -133,3 +133,8 @@ void hfp_hf_unref(struct hfp_hf *hfp);
 bool hfp_hf_set_debug(struct hfp_hf *hfp, hfp_debug_func_t callback,
 				void *user_data, hfp_destroy_func_t destroy);
 bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close);
+bool hfp_hf_set_disconnect_handler(struct hfp_hf *hfp,
+					hfp_disconnect_func_t callback,
+					void *user_data,
+					hfp_destroy_func_t destroy);
+bool hfp_hf_disconnect(struct hfp_hf *hfp);
-- 
1.8.4

[PATCH v4 04/11] shared/hfp: Add register/unregister event for HFP HF

[Lukasz Rymanowski]

This patch adds API which allows to register/unregister for unsolicited
responses.
---
 src/shared/hfp.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/hfp.h |   8 +++++
 2 files changed, 116 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index b7855ed..b1cf08e 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -70,6 +70,8 @@ struct hfp_hf {
 	struct ringbuf *read_buf;
 	struct ringbuf *write_buf;
 
+	struct queue *event_handlers;
+
 	hfp_debug_func_t debug_callback;
 	hfp_destroy_func_t debug_destroy;
 	void *debug_data;
@@ -94,6 +96,18 @@ struct hfp_gw_result {
 	unsigned int offset;
 };
 
+struct hfp_hf_result {
+	const char *data;
+	unsigned int offset;
+};
+
+struct event_handler {
+	char *prefix;
+	void *user_data;
+	hfp_destroy_func_t destroy;
+	hfp_hf_result_func_t callback;
+};
+
 static void destroy_cmd_handler(void *data)
 {
 	struct cmd_handler *handler = data;
@@ -828,6 +842,32 @@ bool hfp_gw_disconnect(struct hfp_gw *hfp)
 	return io_shutdown(hfp->io);
 }
 
+static bool match_handler_event_prefix(const void *a, const void *b)
+{
+	const struct event_handler *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 destroy_event_handler(void *data)
+{
+	struct event_handler *handler = data;
+
+	if (handler->destroy)
+		handler->destroy(handler->user_data);
+
+	free(handler->prefix);
+
+	free(handler);
+}
+
 struct hfp_hf *hfp_hf_new(int fd)
 {
 	struct hfp_hf *hfp;
@@ -863,6 +903,15 @@ struct hfp_hf *hfp_hf_new(int fd)
 		return NULL;
 	}
 
+	hfp->event_handlers = queue_new();
+	if (!hfp->event_handlers) {
+		io_destroy(hfp->io);
+		ringbuf_free(hfp->write_buf);
+		ringbuf_free(hfp->read_buf);
+		free(hfp);
+		return NULL;
+	}
+
 	return hfp_hf_ref(hfp);
 }
 
@@ -902,6 +951,9 @@ void hfp_hf_unref(struct hfp_hf *hfp)
 	ringbuf_free(hfp->write_buf);
 	hfp->write_buf = NULL;
 
+	queue_destroy(hfp->event_handlers, destroy_event_handler);
+	hfp->event_handlers = NULL;
+
 	if (!hfp->in_disconnect) {
 		free(hfp);
 		return;
@@ -961,6 +1013,62 @@ bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close)
 	return true;
 }
 
+bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
+						const char *prefix,
+						void *user_data,
+						hfp_destroy_func_t destroy)
+{
+	struct event_handler *handler;
+
+	if (!callback)
+		return false;
+
+	handler = new0(struct event_handler, 1);
+	if (!handler)
+		return false;
+
+	handler->callback = callback;
+	handler->user_data = user_data;
+
+	handler->prefix = strdup(prefix);
+	if (!handler->prefix) {
+		free(handler);
+		return false;
+	}
+
+	if (queue_find(hfp->event_handlers, match_handler_event_prefix,
+							handler->prefix)) {
+		destroy_event_handler(handler);
+		return false;
+	}
+
+	handler->destroy = destroy;
+
+	return queue_push_tail(hfp->event_handlers, handler);
+}
+
+bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix)
+{
+	struct cmd_handler *handler;
+	char *lookup_prefix;
+
+	lookup_prefix = strdup(prefix);
+	if (!lookup_prefix)
+		return false;
+
+	handler = queue_remove_if(hfp->event_handlers,
+						match_handler_event_prefix,
+						lookup_prefix);
+	free(lookup_prefix);
+
+	if (!handler)
+		return false;
+
+	destroy_event_handler(handler);
+
+	return true;
+}
+
 static void hf_disconnect_watch_destroy(void *user_data)
 {
 	struct hfp_hf *hfp = user_data;
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index a9a169b..85037b1 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -68,10 +68,14 @@ enum hfp_gw_cmd_type {
 };
 
 struct hfp_gw_result;
+struct hfp_hf_result;
 
 typedef void (*hfp_result_func_t)(struct hfp_gw_result *result,
 				enum hfp_gw_cmd_type type, void *user_data);
 
+typedef void (*hfp_hf_result_func_t)(struct hfp_hf_result *result,
+							void *user_data);
+
 typedef void (*hfp_destroy_func_t)(void *user_data);
 typedef void (*hfp_debug_func_t)(const char *str, void *user_data);
 
@@ -138,3 +142,7 @@ bool hfp_hf_set_disconnect_handler(struct hfp_hf *hfp,
 					void *user_data,
 					hfp_destroy_func_t destroy);
 bool hfp_hf_disconnect(struct hfp_hf *hfp);
+bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
+					const char *prefix, void *user_data,
+					hfp_destroy_func_t destroy);
+bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix);
-- 
1.8.4

[PATCH v4 05/11] shared/hfp: Add HFP HF parser

[Lukasz Rymanowski]

This patch adds parser for AT responses and unsolicited results.
---
 src/shared/hfp.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index b1cf08e..5179092 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -868,6 +868,126 @@ static void destroy_event_handler(void *data)
 	free(handler);
 }
 
+static void hf_skip_whitespace(struct hfp_hf_result *result)
+{
+	while (result->data[result->offset] == ' ')
+		result->offset++;
+}
+
+static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
+{
+	struct event_handler *handler;
+	const char *separators = ";:\0";
+	struct hfp_hf_result result_data;
+	char lookup_prefix[18];
+	uint8_t pref_len = 0;
+	const char *prefix;
+	int i;
+
+	result_data.offset = 0;
+	result_data.data = data;
+
+	hf_skip_whitespace(&result_data);
+
+	if (strlen(data + result_data.offset) < 2)
+		return;
+
+	prefix = data + result_data.offset;
+
+	pref_len = strcspn(prefix, separators);
+	if (pref_len > 17 || pref_len < 2)
+		return;
+
+	for (i = 0; i < pref_len; i++)
+		lookup_prefix[i] = toupper(prefix[i]);
+
+	lookup_prefix[pref_len] = '\0';
+	result_data.offset += pref_len + 1;
+
+	handler = queue_find(hfp->event_handlers, match_handler_event_prefix,
+								lookup_prefix);
+	if (!handler)
+		return;
+
+	handler->callback(&result_data, handler->user_data);
+}
+
+static char *find_cr_lf(char *str, size_t len)
+{
+	char *ptr;
+	int count;
+	int offset;
+
+	offset = 0;
+
+	ptr = memchr(str, '\r', len);
+	while (ptr) {
+		/*
+		 * Check if there is more data after '\r'. If so check for
+		 * '\n'
+		 */
+		count = ptr-str;
+		if ((count < (int) (len - 1)) && *(ptr + 1) == '\n')
+			return ptr;
+
+		/* There is only '\r'? Let's try to find next one */
+		offset += count + 1;
+
+		if (offset >= (int)len)
+			return NULL;
+
+		ptr = memchr(str + offset, '\r', len - offset);
+	}
+
+	return NULL;
+}
+
+static void hf_process_input(struct hfp_hf *hfp)
+{
+	char *str, *ptr;
+	size_t len, count, offset;
+
+	str = ringbuf_peek(hfp->read_buf, 0, &len);
+	if (!str)
+		return;
+
+	offset = 0;
+
+	ptr = find_cr_lf(str, len);
+	while (ptr) {
+		count = ptr - (str + offset);
+		if (count == 0) {
+			/* 2 is for <cr><lf> */
+			offset += 2;
+		} else {
+			*ptr = '\0';
+			hf_call_prefix_handler(hfp, str + offset);
+			offset += count + 2;
+		}
+
+		if (offset >= len)
+			break;
+
+		ptr = find_cr_lf(str + offset, len - offset);
+	}
+
+	ringbuf_drain(hfp->read_buf, offset < len ? offset : len);
+}
+
+static bool hf_can_read_data(struct io *io, void *user_data)
+{
+	struct hfp_hf *hfp = user_data;
+	ssize_t bytes_read;
+
+	bytes_read = ringbuf_read(hfp->read_buf, hfp->fd);
+	if (bytes_read < 0)
+		return false;
+
+	hf_process_input(hfp);
+
+	return true;
+}
+
 struct hfp_hf *hfp_hf_new(int fd)
 {
 	struct hfp_hf *hfp;
@@ -912,6 +1032,15 @@ struct hfp_hf *hfp_hf_new(int fd)
 		return NULL;
 	}
 
+	if (!io_set_read_handler(hfp->io, hf_can_read_data, hfp,
+							read_watch_destroy)) {
+		queue_destroy(hfp->event_handlers,
+						destroy_event_handler);
+		io_destroy(hfp->io);
+		ringbuf_free(hfp->write_buf);
+		ringbuf_free(hfp->read_buf);
+	}
+
 	return hfp_hf_ref(hfp);
 }
 
-- 
1.8.4

[PATCH v4 06/11] shared/hfp: Add send AT command API for HFP HF

[Lukasz Rymanowski]

This patch adds handling send and response of AT command.
Note that we always wait for AT command response before sending next
command, however user can fill hfp_hf with more than one command.
All the commands are queued and send one by one.
---
 src/shared/hfp.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/hfp.h |   8 +++
 2 files changed, 183 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 5179092..8bebe97 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -70,6 +70,9 @@ struct hfp_hf {
 	struct ringbuf *read_buf;
 	struct ringbuf *write_buf;
 
+	bool writer_active;
+	struct queue *cmd_queue;
+
 	struct queue *event_handlers;
 
 	hfp_debug_func_t debug_callback;
@@ -101,6 +104,14 @@ struct hfp_hf_result {
 	unsigned int offset;
 };
 
+struct cmd_response {
+	char *prefix;
+	hfp_response_func_t resp_cb;
+	struct hfp_hf_result *response;
+	char *resp_data;
+	void *user_data;
+};
+
 struct event_handler {
 	char *prefix;
 	void *user_data;
@@ -868,17 +879,103 @@ static void destroy_event_handler(void *data)
 	free(handler);
 }
 
+static bool hf_can_write_data(struct io *io, void *user_data)
+{
+	struct hfp_hf *hfp = user_data;
+	ssize_t bytes_written;
+
+	bytes_written = ringbuf_write(hfp->write_buf, hfp->fd);
+	if (bytes_written < 0)
+		return false;
+
+	if (ringbuf_len(hfp->write_buf) > 0)
+		return true;
+
+	return false;
+}
+
+static void hf_write_watch_destroy(void *user_data)
+{
+	struct hfp_hf *hfp = user_data;
+
+	hfp->writer_active = false;
+}
+
 static void hf_skip_whitespace(struct hfp_hf_result *result)
 {
 	while (result->data[result->offset] == ' ')
 		result->offset++;
 }
 
+static bool is_response(const char *prefix, enum hfp_result *result)
+{
+	if (strcmp(prefix, "OK") == 0) {
+		*result = HFP_RESULT_OK;
+		return true;
+	}
+
+	if (strcmp(prefix, "ERROR") == 0) {
+		*result = HFP_RESULT_ERROR;
+		return true;
+	}
+
+	if (strcmp(prefix, "NO CARRIER") == 0) {
+		*result = HFP_RESULT_NO_CARRIER;
+		return true;
+	}
+
+	if (strcmp(prefix, "CONNECT") == 0) {
+		*result = HFP_RESULT_CONNECT;
+		return true;
+	}
+
+	if (strcmp(prefix, "NO ANSWER") == 0) {
+		*result = HFP_RESULT_NO_ANSWER;
+		return true;
+	}
+
+	if (strcmp(prefix, "DELAYED") == 0) {
+		*result = HFP_RESULT_DELAYED;
+		return true;
+	}
+
+	if (strcmp(prefix, "BLACKLISTED") == 0) {
+		*result = HFP_RESULT_BLACKLISTED;
+		return true;
+	}
+
+	return false;
+}
+
+static void hf_wakeup_writer(struct hfp_hf *hfp)
+{
+	if (hfp->writer_active)
+		return;
+
+	if (!ringbuf_len(hfp->write_buf))
+		return;
+
+	if (!io_set_write_handler(hfp->io, hf_can_write_data,
+					hfp, hf_write_watch_destroy))
+		return;
+
+	hfp->writer_active = true;
+}
+
+static void destroy_cmd_response(void *data)
+{
+	struct cmd_response *cmd = data;
+
+	free(cmd->prefix);
+	free(cmd);
+}
+
 static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
 {
 	struct event_handler *handler;
 	const char *separators = ";:\0";
 	struct hfp_hf_result result_data;
+	enum hfp_result result;
 	char lookup_prefix[18];
 	uint8_t pref_len = 0;
 	const char *prefix;
@@ -904,6 +1001,22 @@ static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
 	lookup_prefix[pref_len] = '\0';
 	result_data.offset += pref_len + 1;
 
+	if (is_response(lookup_prefix, &result)) {
+		struct cmd_response *cmd;
+
+		cmd = queue_peek_head(hfp->cmd_queue);
+		if (!cmd)
+			return;
+
+		cmd->resp_cb(cmd->prefix, result, cmd->user_data);
+
+		queue_remove(hfp->cmd_queue, cmd);
+		destroy_cmd_response(cmd);
+
+		hf_wakeup_writer(hfp);
+		return;
+	}
+
 	handler = queue_find(hfp->event_handlers, match_handler_event_prefix,
 								lookup_prefix);
 	if (!handler)
@@ -1032,6 +1145,18 @@ struct hfp_hf *hfp_hf_new(int fd)
 		return NULL;
 	}
 
+	hfp->cmd_queue = queue_new();
+	if (!hfp->cmd_queue) {
+		io_destroy(hfp->io);
+		ringbuf_free(hfp->write_buf);
+		ringbuf_free(hfp->read_buf);
+		queue_destroy(hfp->event_handlers, NULL);
+		free(hfp);
+		return NULL;
+	}
+
+	hfp->writer_active = false;
+
 	if (!io_set_read_handler(hfp->io, hf_can_read_data, hfp,
 							read_watch_destroy)) {
 		queue_destroy(hfp->event_handlers,
@@ -1083,6 +1208,9 @@ void hfp_hf_unref(struct hfp_hf *hfp)
 	queue_destroy(hfp->event_handlers, destroy_event_handler);
 	hfp->event_handlers = NULL;
 
+	queue_destroy(hfp->cmd_queue, destroy_cmd_response);
+	hfp->cmd_queue = NULL;
+
 	if (!hfp->in_disconnect) {
 		free(hfp);
 		return;
@@ -1142,6 +1270,53 @@ bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close)
 	return true;
 }
 
+bool hfp_hf_send_command(struct hfp_hf *hfp, hfp_response_func_t resp_cb,
+				void *user_data, const char *format, ...)
+{
+	va_list ap;
+	char *fmt;
+	int len;
+	const char *separators = ";?=\0";
+	uint8_t prefix_len;
+	struct cmd_response *cmd;
+
+	if (!hfp || !format || !resp_cb)
+		return false;
+
+	if (asprintf(&fmt, "%s\r", format) < 0)
+		return false;
+
+	cmd = new0(struct cmd_response, 1);
+	if (!cmd)
+		return false;
+
+	va_start(ap, format);
+	len = ringbuf_vprintf(hfp->write_buf, fmt, ap);
+	va_end(ap);
+
+	free(fmt);
+
+	if (len < 0) {
+		free(cmd);
+		return false;
+	}
+
+	prefix_len = strcspn(format, separators);
+	cmd->prefix = strndup(format, prefix_len);
+	cmd->resp_cb = resp_cb;
+	cmd->user_data = user_data;
+
+	if (!queue_push_tail(hfp->cmd_queue, cmd)) {
+		ringbuf_drain(hfp->write_buf, len);
+		free(cmd);
+		return false;
+	}
+
+	hf_wakeup_writer(hfp);
+
+	return true;
+}
+
 bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
 						const char *prefix,
 						void *user_data,
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index 85037b1..5ee3017 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -32,6 +32,8 @@ enum hfp_result {
 	HFP_RESULT_NO_DIALTONE	= 6,
 	HFP_RESULT_BUSY		= 7,
 	HFP_RESULT_NO_ANSWER	= 8,
+	HFP_RESULT_DELAYED	= 9,
+	HFP_RESULT_BLACKLISTED	= 10,
 };
 
 enum hfp_error {
@@ -83,6 +85,10 @@ typedef void (*hfp_command_func_t)(const char *command, void *user_data);
 
 typedef void (*hfp_disconnect_func_t)(void *user_data);
 
+typedef void (*hfp_response_func_t)(const char *prefix,
+							enum hfp_result result,
+							void *user_data);
+
 struct hfp_gw;
 struct hfp_hf;
 
@@ -146,3 +152,5 @@ bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
 					const char *prefix, void *user_data,
 					hfp_destroy_func_t destroy);
 bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix);
+bool hfp_hf_send_command(struct hfp_hf *hfp, hfp_response_func_t resp_cb,
+				void *user_data, const char *format, ...);
-- 
1.8.4

[PATCH v4 07/11] unit/test-hfp: Provide test_handler function via struct data

[Lukasz Rymanowski]

This patch allows us to use user defined test handler depends on needs.
Will use it in following patches which implements tests for HFP HF.
---
 unit/test-hfp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/unit/test-hfp.c b/unit/test-hfp.c
index a8801b0..4b3473b 100644
--- a/unit/test-hfp.c
+++ b/unit/test-hfp.c
@@ -52,6 +52,7 @@ struct test_data {
 	char *test_name;
 	struct test_pdu *pdu_list;
 	hfp_result_func_t result_func;
+	GIOFunc test_handler;
 };
 
 #define data(args...) ((const unsigned char[]) { args })
@@ -95,6 +96,7 @@ struct test_data {
 		data.result_func = result_function;			\
 		memcpy(data.pdu_list, pdus, sizeof(pdus));		\
 		g_test_add_data_func(name, &data, function);		\
+		data.test_handler = test_handler;			\
 	} while (0)
 
 static void context_quit(struct context *context)
@@ -158,6 +160,7 @@ static struct context *create_context(gconstpointer data)
 	struct context *context = g_new0(struct context, 1);
 	GIOChannel *channel;
 	int err, sv[2];
+	const struct test_data *d = data;
 
 	context->main_loop = g_main_loop_new(NULL, FALSE);
 	g_assert(context->main_loop);
@@ -173,7 +176,8 @@ static struct context *create_context(gconstpointer data)
 
 	context->watch_id = g_io_add_watch(channel,
 				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
-				test_handler, context);
+				d->test_handler, context);
+
 	g_assert(context->watch_id > 0);
 
 	g_io_channel_unref(channel);
-- 
1.8.4

[PATCH v4 08/11] unit/test-hfp: Add init test for HFP HF

[Lukasz Rymanowski]

This patch adds basic infrastruction for HFP HF test plus
init test.

It also moves send_pdu function in the file so it can be used by
test_hf_handler
---
 unit/test-hfp.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 84 insertions(+), 18 deletions(-)

diff --git a/unit/test-hfp.c b/unit/test-hfp.c
index 4b3473b..274ee55 100644
--- a/unit/test-hfp.c
+++ b/unit/test-hfp.c
@@ -36,6 +36,7 @@ struct context {
 	int fd_server;
 	int fd_client;
 	struct hfp_gw *hfp;
+	struct hfp_hf *hfp_hf;
 	const struct test_data *data;
 	unsigned int pdu_offset;
 };
@@ -52,6 +53,8 @@ struct test_data {
 	char *test_name;
 	struct test_pdu *pdu_list;
 	hfp_result_func_t result_func;
+	hfp_response_func_t response_func;
+	hfp_hf_result_func_t hf_result_func;
 	GIOFunc test_handler;
 };
 
@@ -99,6 +102,22 @@ struct test_data {
 		data.test_handler = test_handler;			\
 	} while (0)
 
+#define define_hf_test(name, function, result_func, response_function,	\
+								args...)\
+	do {								\
+		const struct test_pdu pdus[] = {			\
+			args, { }					\
+		};							\
+		static struct test_data data;				\
+		data.test_name = g_strdup(name);			\
+		data.pdu_list = g_malloc(sizeof(pdus));			\
+		data.hf_result_func = result_func;			\
+		data.response_func = response_function;			\
+		memcpy(data.pdu_list, pdus, sizeof(pdus));		\
+		g_test_add_data_func(name, &data, function);		\
+		data.test_handler = test_hf_handler;			\
+	} while (0)
+
 static void context_quit(struct context *context)
 {
 	g_main_loop_quit(context->main_loop);
@@ -128,6 +147,52 @@ static gboolean test_handler(GIOChannel *channel, GIOCondition cond,
 	return FALSE;
 }
 
+static gboolean send_pdu(gpointer user_data)
+{
+	struct context *context = user_data;
+	const struct test_pdu *pdu;
+	ssize_t len;
+
+	pdu = &context->data->pdu_list[context->pdu_offset++];
+
+	if (pdu && !pdu->valid)
+		return FALSE;
+
+	len = write(context->fd_server, pdu->data, pdu->size);
+	g_assert_cmpint(len, ==, pdu->size);
+
+	pdu = &context->data->pdu_list[context->pdu_offset];
+	if (pdu->fragmented)
+		g_idle_add(send_pdu, context);
+
+	return FALSE;
+}
+
+static gboolean test_hf_handler(GIOChannel *channel, GIOCondition cond,
+							gpointer user_data)
+{
+	struct context *context = user_data;
+	gchar buf[60];
+	gsize bytes_read;
+	GError *error = NULL;
+
+	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
+		goto done;
+
+	/* dummy read */
+	g_io_channel_read_chars(channel, buf, 60, &bytes_read, &error);
+
+	send_pdu(context);
+
+	return TRUE;
+
+done:
+	context_quit(context);
+	context->watch_id = 0;
+
+	return FALSE;
+}
+
 static void cmd_handler(const char *command, void *user_data)
 {
 	struct context *context = user_data;
@@ -203,6 +268,9 @@ static void execute_context(struct context *context)
 	if (context->hfp)
 		hfp_gw_unref(context->hfp);
 
+	if (context->hfp_hf)
+		hfp_hf_unref(context->hfp_hf);
+
 	g_free(context);
 }
 
@@ -275,24 +343,6 @@ static void test_register(gconstpointer data)
 	execute_context(context);
 }
 
-static gboolean send_pdu(gpointer user_data)
-{
-	struct context *context = user_data;
-	const struct test_pdu *pdu;
-	ssize_t len;
-
-	pdu = &context->data->pdu_list[context->pdu_offset++];
-
-	len = write(context->fd_server, pdu->data, pdu->size);
-	g_assert_cmpint(len, ==, pdu->size);
-
-	pdu = &context->data->pdu_list[context->pdu_offset];
-	if (pdu->fragmented)
-		g_idle_add(send_pdu, context);
-
-	return FALSE;
-}
-
 static void test_fragmented(gconstpointer data)
 {
 	struct context *context = create_context(data);
@@ -404,6 +454,20 @@ static void check_string_2(struct hfp_gw_result *result,
 	hfp_gw_send_result(context->hfp, HFP_RESULT_ERROR);
 }
 
+static void test_hf_init(gconstpointer data)
+{
+	struct context *context = create_context(data);
+
+	context->hfp_hf = hfp_hf_new(context->fd_client);
+	g_assert(context->hfp_hf);
+	g_assert(hfp_hf_set_close_on_unref(context->hfp_hf, true));
+
+	hfp_hf_unref(context->hfp_hf);
+	context->hfp_hf = NULL;
+
+	execute_context(context);
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -473,5 +537,7 @@ int main(int argc, char *argv[])
 			raw_pdu('\r'),
 			data_end());
 
+	define_hf_test("/hfp/test_init", test_hf_init, NULL, NULL, data_end());
+
 	return g_test_run();
 }
-- 
1.8.4

[PATCH v4 09/11] unit/test-hfp: Add send command tests for HFP HF

[Lukasz Rymanowski]

This patch adds following tests:
/hfp/test_send_command_1
/hfp/test_send_command_2
---
 unit/test-hfp.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/unit/test-hfp.c b/unit/test-hfp.c
index 274ee55..34273d5 100644
--- a/unit/test-hfp.c
+++ b/unit/test-hfp.c
@@ -468,6 +468,68 @@ static void test_hf_init(gconstpointer data)
 	execute_context(context);
 }
 
+static bool unsolicited_resp = false;
+
+static void hf_unsolicited_resp_cb(struct hfp_hf_result *result,
+							void *user_data) {
+	unsolicited_resp = true;
+}
+
+static void hf_response_with_data(const char *prefix, enum hfp_result res,
+							void *user_data)
+{
+	struct context *context = user_data;
+
+	g_assert(unsolicited_resp);
+	unsolicited_resp = false;
+
+	hfp_hf_disconnect(context->hfp_hf);
+}
+
+static void hf_brsf_response_cb(const char *prefix, enum hfp_result res,
+							void *user_data)
+{
+	struct context *context = user_data;
+
+	g_assert_cmpstr(prefix, ==, "AT+BRSF");
+
+	hfp_hf_disconnect(context->hfp_hf);
+}
+
+static void test_hf_send_command(gconstpointer data)
+{
+	struct context *context = create_context(data);
+	const struct test_pdu *pdu;
+	bool ret;
+
+	context->hfp_hf = hfp_hf_new(context->fd_client);
+	g_assert(context->hfp_hf);
+
+	ret = hfp_hf_set_close_on_unref(context->hfp_hf, true);
+	g_assert(ret);
+
+	if (context->data->response_func) {
+		if (context->data->hf_result_func) {
+			pdu = &context->data->pdu_list[context->pdu_offset++];
+
+			ret = hfp_hf_register(context->hfp_hf,
+						context->data->hf_result_func,
+						(char *)pdu->data,
+						NULL, NULL);
+			g_assert(ret);
+		}
+
+		pdu = &context->data->pdu_list[context->pdu_offset++];
+
+		ret = hfp_hf_send_command(context->hfp_hf,
+						context->data->response_func,
+						context, (char *)pdu->data);
+		g_assert(ret);
+	}
+
+	execute_context(context);
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -538,6 +600,21 @@ int main(int argc, char *argv[])
 			data_end());
 
 	define_hf_test("/hfp/test_init", test_hf_init, NULL, NULL, data_end());
+	define_hf_test("/hfp/test_send_command_1", test_hf_send_command, NULL,
+			hf_brsf_response_cb,
+			raw_pdu('A', 'T', '+', 'B', 'R', 'S', 'F', '\0'),
+			raw_pdu('\r', '\n', 'O', 'k', '\r', '\n'),
+			data_end());
+
+	define_hf_test("/hfp/test_send_command_2", test_hf_send_command,
+			hf_unsolicited_resp_cb,
+			hf_response_with_data,
+			raw_pdu('+', 'B', 'R', 'S', 'F', '\0'),
+			raw_pdu('A', 'T', '+', 'B', 'R', 'S', 'F', '\0'),
+			frg_pdu('\r', '\n', '+', 'B', 'R', 'S', 'F', '\r',
+									'\n'),
+			frg_pdu('\r', '\n', 'O', 'k', '\r', '\n'),
+			data_end());
 
 	return g_test_run();
 }
-- 
1.8.4

[PATCH v4 10/11] unit/test-hfp: Add tests for unsolicited results for HFP HF

[Lukasz Rymanowski]

This patch adds three test case:
/hfp/test_unsolicited_1
/hfp/test_unsolicited_2
/hfp/test_unsolicited_3
---
 unit/test-hfp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/unit/test-hfp.c b/unit/test-hfp.c
index 34273d5..266f8cd 100644
--- a/unit/test-hfp.c
+++ b/unit/test-hfp.c
@@ -530,6 +530,42 @@ static void test_hf_send_command(gconstpointer data)
 	execute_context(context);
 }
 
+static void hf_result_handler(struct hfp_hf_result *result,
+							void *user_data)
+{
+	struct context *context = user_data;
+
+	hfp_hf_disconnect(context->hfp_hf);
+}
+
+static void test_hf_unsolicited(gconstpointer data)
+{
+	struct context *context = create_context(data);
+	bool ret;
+
+	context->hfp_hf = hfp_hf_new(context->fd_client);
+	g_assert(context->hfp_hf);
+
+	ret = hfp_hf_set_close_on_unref(context->hfp_hf, true);
+	g_assert(ret);
+
+	if (context->data->hf_result_func) {
+		const struct test_pdu *pdu;
+
+		pdu = &context->data->pdu_list[context->pdu_offset++];
+
+		ret = hfp_hf_register(context->hfp_hf,
+						context->data->hf_result_func,
+						(char *)pdu->data, context,
+						NULL);
+		g_assert(ret);
+	}
+
+	send_pdu(context);
+
+	execute_context(context);
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -616,5 +652,29 @@ int main(int argc, char *argv[])
 			frg_pdu('\r', '\n', 'O', 'k', '\r', '\n'),
 			data_end());
 
+	define_hf_test("/hfp/test_unsolicited_1", test_hf_unsolicited,
+			hf_result_handler, NULL,
+			raw_pdu('+', 'C', 'L', 'C', 'C', '\0'),
+			frg_pdu('\r', '\n', '+', 'C', 'L', 'C'),
+			frg_pdu('C', '\r', '\n'),
+			data_end());
+
+	define_hf_test("/hfp/test_unsolicited_2", test_hf_unsolicited,
+			hf_result_handler, NULL,
+			raw_pdu('+', 'C', 'L', 'C', 'C', '\0'),
+			frg_pdu('\r', '\n', '+', 'C', 'L', 'C', 'C', ':', '1'),
+			frg_pdu(',', '3', ',', '0', '\r', '\n'),
+			data_end());
+
+	define_hf_test("/hfp/test_unsolicited_3", test_hf_unsolicited,
+			hf_result_handler, NULL,
+			raw_pdu('+', 'C', 'L', 'C', 'C', '\0'),
+			frg_pdu('\r'), frg_pdu('\n'), frg_pdu('+'),
+			frg_pdu('C'), frg_pdu('L'), frg_pdu('C'), frg_pdu('C'),
+			frg_pdu(':'), frg_pdu('1'), frg_pdu(','), frg_pdu('3'),
+			frg_pdu(','), frg_pdu('0'), frg_pdu('\r'),
+			frg_pdu('\n'),
+			data_end());
+
 	return g_test_run();
 }
-- 
1.8.4

[PATCH v4 11/11] unit/test-hfp: Add some robustness tests for HFP HF

[Lukasz Rymanowski]

This patch adds folowing tests:
/hfp/test_hf_corrupted_1
/hfp/test_hf_corrupted_2
/hfp/test_hf_empty
/hfp/test_hf_unknown
---
 unit/test-hfp.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/unit/test-hfp.c b/unit/test-hfp.c
index 266f8cd..2515f26 100644
--- a/unit/test-hfp.c
+++ b/unit/test-hfp.c
@@ -566,6 +566,25 @@ static void test_hf_unsolicited(gconstpointer data)
 	execute_context(context);
 }
 
+static void test_hf_robustness(gconstpointer data)
+{
+	struct context *context = create_context(data);
+	bool ret;
+
+	context->hfp_hf = hfp_hf_new(context->fd_client);
+	g_assert(context->hfp_hf);
+
+	ret = hfp_hf_set_close_on_unref(context->hfp_hf, true);
+	g_assert(ret);
+
+	send_pdu(context);
+
+	hfp_hf_unref(context->hfp_hf);
+	context->hfp_hf = NULL;
+
+	execute_context(context);
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -676,5 +695,26 @@ int main(int argc, char *argv[])
 			frg_pdu('\n'),
 			data_end());
 
+	define_hf_test("/hfp/test_hf_corrupted_1", test_hf_unsolicited,
+			hf_result_handler, NULL,
+			raw_pdu('+', 'C', 'L', 'C', 'C', '\0'),
+			frg_pdu('\r', 'X', '\r', '\n'),
+			frg_pdu('+', 'C', 'L', 'C', 'C', ':', '1', ',', '3'),
+			frg_pdu(',', '0', '\r', '\n'),
+			data_end());
+
+	define_hf_test("/hfp/test_hf_corrupted_2", test_hf_unsolicited,
+			hf_result_handler, NULL,
+			raw_pdu('+', 'C', 'L', 'C', 'C', '\0'),
+			raw_pdu('+', 'C', 'L', 'C', 'C', '\r', '\n'),
+			data_end());
+
+	define_hf_test("/hfp/test_hf_empty", test_hf_robustness, NULL, NULL,
+			raw_pdu('\r'), data_end());
+
+	define_hf_test("/hfp/test_hf_unknown", test_hf_robustness, NULL, NULL,
+			raw_pdu('\r', '\n', 'B', 'R', '\r', '\n'),
+			data_end());
+
 	return g_test_run();
 }
-- 
1.8.4

Re: [PATCH v4 00/11] shared/hfp: Add support for HFP HF

[Lukasz Rymanowski]

Hi,

On 10 October 2014 01:50, Lukasz Rymanowski <lukasz.rymanowski@tieto.com> wrote:
> Following patches extends hfp API with HFP HF functionality.
> HFP HF parser has been added and unit test for it.
>
> To consider: how strict we should be when it comes to parsing
> AT responses. For example, at the moment, command +CCLC:<cr><lf>
> will be recognized as +CCLC: eventhough correct response format
> should be <cr><lf>+CCLC:<cr><lf>
>
> Note: As discussed on IRC I did not try to generalize code.
>
> v2:
> * minor self review fixes
> * response callback on send command, contains now result (OK/ERROR) and
> data from unsolicited response if available.
>
> v3:
> * Fix some memory leaks found on self review
>
> v4:
> * Fallback to approach from v1 in context of response callback for AT command.
> Bassically, if AT+X has +X and OK response, response callback contains only OK or
> ERROR code (including CME which will be added in following patches). To get +X
> response, user need to use hfp_hf_register() API. It is done mostly to keep hfp.c
> simple. With this approach we do not have to cache all +X in hfp.c before calling
> response callback.
>
> Lukasz Rymanowski (11):
>   shared/hfp: Add support for HFP HF
>   shared/hfp: Add set_debug and close_on_unref API for HFP HF
>   shared/hfp: Add set disconnect handler and disconnect API to HFP HF
>   shared/hfp: Add register/unregister event for HFP HF
>   shared/hfp: Add HFP HF parser
>   shared/hfp: Add send AT command API for HFP HF
>   unit/test-hfp: Provide test_handler function via struct data
>   unit/test-hfp: Add init test for HFP HF
>   unit/test-hfp: Add send command tests for HFP HF
>   unit/test-hfp: Add tests for unsolicited results for HFP HF
>   unit/test-hfp: Add some robustness tests for HFP HF
>
>  src/shared/hfp.c | 624 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/shared/hfp.h |  30 +++
>  unit/test-hfp.c  | 285 +++++++++++++++++++++++--
>  3 files changed, 920 insertions(+), 19 deletions(-)
>

ping

\Lukasz
> --
> 1.8.4
>

Re: [PATCH v4 08/11] unit/test-hfp: Add init test for HFP HF

[Szymon Janc]

Hi Łukasz,

On Friday 10 of October 2014 01:50:08 Lukasz Rymanowski wrote:
> This patch adds basic infrastruction for HFP HF test plus
> init test.
> 
> It also moves send_pdu function in the file so it can be used by
> test_hf_handler
> ---
>  unit/test-hfp.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 84 insertions(+), 18 deletions(-)
> 
> diff --git a/unit/test-hfp.c b/unit/test-hfp.c
> index 4b3473b..274ee55 100644
> --- a/unit/test-hfp.c
> +++ b/unit/test-hfp.c
> @@ -36,6 +36,7 @@ struct context {
>  	int fd_server;
>  	int fd_client;
>  	struct hfp_gw *hfp;
> +	struct hfp_hf *hfp_hf;
>  	const struct test_data *data;
>  	unsigned int pdu_offset;
>  };
> @@ -52,6 +53,8 @@ struct test_data {
>  	char *test_name;
>  	struct test_pdu *pdu_list;
>  	hfp_result_func_t result_func;
> +	hfp_response_func_t response_func;
> +	hfp_hf_result_func_t hf_result_func;
>  	GIOFunc test_handler;
>  };
>  
> @@ -99,6 +102,22 @@ struct test_data {
>  		data.test_handler = test_handler;			\
>  	} while (0)
>  
> +#define define_hf_test(name, function, result_func, response_function,	\
> +								args...)\
> +	do {								\
> +		const struct test_pdu pdus[] = {			\
> +			args, { }					\
> +		};							\
> +		static struct test_data data;				\
> +		data.test_name = g_strdup(name);			\
> +		data.pdu_list = g_malloc(sizeof(pdus));			\
> +		data.hf_result_func = result_func;			\
> +		data.response_func = response_function;			\
> +		memcpy(data.pdu_list, pdus, sizeof(pdus));		\
> +		g_test_add_data_func(name, &data, function);		\
> +		data.test_handler = test_hf_handler;			\
> +	} while (0)
> +
>  static void context_quit(struct context *context)
>  {
>  	g_main_loop_quit(context->main_loop);
> @@ -128,6 +147,52 @@ static gboolean test_handler(GIOChannel *channel, GIOCondition cond,
>  	return FALSE;
>  }
>  
> +static gboolean send_pdu(gpointer user_data)
> +{
> +	struct context *context = user_data;
> +	const struct test_pdu *pdu;
> +	ssize_t len;
> +
> +	pdu = &context->data->pdu_list[context->pdu_offset++];
> +
> +	if (pdu && !pdu->valid)
> +		return FALSE;
> +
> +	len = write(context->fd_server, pdu->data, pdu->size);
> +	g_assert_cmpint(len, ==, pdu->size);
> +
> +	pdu = &context->data->pdu_list[context->pdu_offset];
> +	if (pdu->fragmented)
> +		g_idle_add(send_pdu, context);
> +
> +	return FALSE;
> +}
> +
> +static gboolean test_hf_handler(GIOChannel *channel, GIOCondition cond,
> +							gpointer user_data)
> +{
> +	struct context *context = user_data;
> +	gchar buf[60];
> +	gsize bytes_read;
> +	GError *error = NULL;
> +
> +	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> +		goto done;
> +
> +	/* dummy read */
> +	g_io_channel_read_chars(channel, buf, 60, &bytes_read, &error);
> +
> +	send_pdu(context);
> +
> +	return TRUE;
> +
> +done:
> +	context_quit(context);
> +	context->watch_id = 0;
> +
> +	return FALSE;
> +}
> +
>  static void cmd_handler(const char *command, void *user_data)
>  {
>  	struct context *context = user_data;
> @@ -203,6 +268,9 @@ static void execute_context(struct context *context)
>  	if (context->hfp)
>  		hfp_gw_unref(context->hfp);
>  
> +	if (context->hfp_hf)
> +		hfp_hf_unref(context->hfp_hf);
> +
>  	g_free(context);
>  }
>  
> @@ -275,24 +343,6 @@ static void test_register(gconstpointer data)
>  	execute_context(context);
>  }
>  
> -static gboolean send_pdu(gpointer user_data)
> -{
> -	struct context *context = user_data;
> -	const struct test_pdu *pdu;
> -	ssize_t len;
> -
> -	pdu = &context->data->pdu_list[context->pdu_offset++];
> -
> -	len = write(context->fd_server, pdu->data, pdu->size);
> -	g_assert_cmpint(len, ==, pdu->size);
> -
> -	pdu = &context->data->pdu_list[context->pdu_offset];
> -	if (pdu->fragmented)
> -		g_idle_add(send_pdu, context);
> -
> -	return FALSE;
> -}
> -
>  static void test_fragmented(gconstpointer data)
>  {
>  	struct context *context = create_context(data);
> @@ -404,6 +454,20 @@ static void check_string_2(struct hfp_gw_result *result,
>  	hfp_gw_send_result(context->hfp, HFP_RESULT_ERROR);
>  }
>  
> +static void test_hf_init(gconstpointer data)
> +{
> +	struct context *context = create_context(data);
> +
> +	context->hfp_hf = hfp_hf_new(context->fd_client);
> +	g_assert(context->hfp_hf);
> +	g_assert(hfp_hf_set_close_on_unref(context->hfp_hf, true));
> +
> +	hfp_hf_unref(context->hfp_hf);
> +	context->hfp_hf = NULL;
> +
> +	execute_context(context);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	g_test_init(&argc, &argv, NULL);
> @@ -473,5 +537,7 @@ int main(int argc, char *argv[])
>  			raw_pdu('\r'),
>  			data_end());
>  
> +	define_hf_test("/hfp/test_init", test_hf_init, NULL, NULL, data_end());

I'd prefer if all hfp_hf tests were prefixed like this:
"/hfp_hf/test_foo"

This will allow to avoid doubling tests name like this one (there is already
/hfp/test_init test).

> +
>  	return g_test_run();
>  }
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH v4 06/11] shared/hfp: Add send AT command API for HFP HF

[Szymon Janc]

Hi Łukasz,

On Friday 10 of October 2014 01:50:06 Lukasz Rymanowski wrote:
> This patch adds handling send and response of AT command.
> Note that we always wait for AT command response before sending next
> command, however user can fill hfp_hf with more than one command.
> All the commands are queued and send one by one.
> ---
>  src/shared/hfp.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/shared/hfp.h |   8 +++
>  2 files changed, 183 insertions(+)
> 
> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
> index 5179092..8bebe97 100644
> --- a/src/shared/hfp.c
> +++ b/src/shared/hfp.c
> @@ -70,6 +70,9 @@ struct hfp_hf {
>  	struct ringbuf *read_buf;
>  	struct ringbuf *write_buf;
>  
> +	bool writer_active;
> +	struct queue *cmd_queue;
> +
>  	struct queue *event_handlers;
>  
>  	hfp_debug_func_t debug_callback;
> @@ -101,6 +104,14 @@ struct hfp_hf_result {
>  	unsigned int offset;
>  };
>  
> +struct cmd_response {
> +	char *prefix;
> +	hfp_response_func_t resp_cb;
> +	struct hfp_hf_result *response;
> +	char *resp_data;
> +	void *user_data;
> +};
> +
>  struct event_handler {
>  	char *prefix;
>  	void *user_data;
> @@ -868,17 +879,103 @@ static void destroy_event_handler(void *data)
>  	free(handler);
>  }
>  
> +static bool hf_can_write_data(struct io *io, void *user_data)
> +{
> +	struct hfp_hf *hfp = user_data;
> +	ssize_t bytes_written;
> +
> +	bytes_written = ringbuf_write(hfp->write_buf, hfp->fd);
> +	if (bytes_written < 0)
> +		return false;
> +
> +	if (ringbuf_len(hfp->write_buf) > 0)
> +		return true;
> +
> +	return false;
> +}
> +
> +static void hf_write_watch_destroy(void *user_data)
> +{
> +	struct hfp_hf *hfp = user_data;
> +
> +	hfp->writer_active = false;
> +}
> +
>  static void hf_skip_whitespace(struct hfp_hf_result *result)
>  {
>  	while (result->data[result->offset] == ' ')
>  		result->offset++;
>  }
>  
> +static bool is_response(const char *prefix, enum hfp_result *result)
> +{
> +	if (strcmp(prefix, "OK") == 0) {
> +		*result = HFP_RESULT_OK;
> +		return true;
> +	}
> +
> +	if (strcmp(prefix, "ERROR") == 0) {
> +		*result = HFP_RESULT_ERROR;
> +		return true;
> +	}
> +
> +	if (strcmp(prefix, "NO CARRIER") == 0) {
> +		*result = HFP_RESULT_NO_CARRIER;
> +		return true;
> +	}
> +
> +	if (strcmp(prefix, "CONNECT") == 0) {

Shouldn't this be "BUSY"?

> +		*result = HFP_RESULT_CONNECT;

And this enum value looks bogus to me.
I couldn't find it in HFP nor HSP spec. Probably leftover from AT spec.
I'd just handle what is defined in HFP spec here.

> +		return true;
> +	}
> +
> +	if (strcmp(prefix, "NO ANSWER") == 0) {
> +		*result = HFP_RESULT_NO_ANSWER;
> +		return true;
> +	}
> +
> +	if (strcmp(prefix, "DELAYED") == 0) {
> +		*result = HFP_RESULT_DELAYED;
> +		return true;
> +	}
> +
> +	if (strcmp(prefix, "BLACKLISTED") == 0) {
> +		*result = HFP_RESULT_BLACKLISTED;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void hf_wakeup_writer(struct hfp_hf *hfp)
> +{
> +	if (hfp->writer_active)
> +		return;
> +
> +	if (!ringbuf_len(hfp->write_buf))
> +		return;
> +
> +	if (!io_set_write_handler(hfp->io, hf_can_write_data,
> +					hfp, hf_write_watch_destroy))
> +		return;
> +
> +	hfp->writer_active = true;
> +}
> +
> +static void destroy_cmd_response(void *data)
> +{
> +	struct cmd_response *cmd = data;
> +
> +	free(cmd->prefix);
> +	free(cmd);
> +}
> +
>  static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
>  {
>  	struct event_handler *handler;
>  	const char *separators = ";:\0";
>  	struct hfp_hf_result result_data;
> +	enum hfp_result result;
>  	char lookup_prefix[18];
>  	uint8_t pref_len = 0;
>  	const char *prefix;
> @@ -904,6 +1001,22 @@ static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
>  	lookup_prefix[pref_len] = '\0';
>  	result_data.offset += pref_len + 1;
>  
> +	if (is_response(lookup_prefix, &result)) {
> +		struct cmd_response *cmd;
> +
> +		cmd = queue_peek_head(hfp->cmd_queue);
> +		if (!cmd)
> +			return;
> +
> +		cmd->resp_cb(cmd->prefix, result, cmd->user_data);
> +
> +		queue_remove(hfp->cmd_queue, cmd);
> +		destroy_cmd_response(cmd);
> +
> +		hf_wakeup_writer(hfp);
> +		return;
> +	}
> +
>  	handler = queue_find(hfp->event_handlers, match_handler_event_prefix,
>  								lookup_prefix);
>  	if (!handler)
> @@ -1032,6 +1145,18 @@ struct hfp_hf *hfp_hf_new(int fd)
>  		return NULL;
>  	}
>  
> +	hfp->cmd_queue = queue_new();
> +	if (!hfp->cmd_queue) {
> +		io_destroy(hfp->io);
> +		ringbuf_free(hfp->write_buf);
> +		ringbuf_free(hfp->read_buf);
> +		queue_destroy(hfp->event_handlers, NULL);
> +		free(hfp);
> +		return NULL;
> +	}
> +
> +	hfp->writer_active = false;
> +
>  	if (!io_set_read_handler(hfp->io, hf_can_read_data, hfp,
>  							read_watch_destroy)) {
>  		queue_destroy(hfp->event_handlers,
> @@ -1083,6 +1208,9 @@ void hfp_hf_unref(struct hfp_hf *hfp)
>  	queue_destroy(hfp->event_handlers, destroy_event_handler);
>  	hfp->event_handlers = NULL;
>  
> +	queue_destroy(hfp->cmd_queue, destroy_cmd_response);
> +	hfp->cmd_queue = NULL;
> +
>  	if (!hfp->in_disconnect) {
>  		free(hfp);
>  		return;
> @@ -1142,6 +1270,53 @@ bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close)
>  	return true;
>  }
>  
> +bool hfp_hf_send_command(struct hfp_hf *hfp, hfp_response_func_t resp_cb,
> +				void *user_data, const char *format, ...)
> +{
> +	va_list ap;
> +	char *fmt;
> +	int len;
> +	const char *separators = ";?=\0";
> +	uint8_t prefix_len;
> +	struct cmd_response *cmd;
> +
> +	if (!hfp || !format || !resp_cb)
> +		return false;
> +
> +	if (asprintf(&fmt, "%s\r", format) < 0)
> +		return false;
> +
> +	cmd = new0(struct cmd_response, 1);
> +	if (!cmd)
> +		return false;
> +
> +	va_start(ap, format);
> +	len = ringbuf_vprintf(hfp->write_buf, fmt, ap);
> +	va_end(ap);
> +
> +	free(fmt);
> +
> +	if (len < 0) {
> +		free(cmd);
> +		return false;
> +	}
> +
> +	prefix_len = strcspn(format, separators);

I'd explore possibility of passing prefix as separate argument to the
function to avoid need of this extra parsing. Also do we really need this
prefix at all? We should not have more than one pending AT command anyway.

> +	cmd->prefix = strndup(format, prefix_len);
> +	cmd->resp_cb = resp_cb;
> +	cmd->user_data = user_data;
> +
> +	if (!queue_push_tail(hfp->cmd_queue, cmd)) {
> +		ringbuf_drain(hfp->write_buf, len);
> +		free(cmd);
> +		return false;
> +	}
> +
> +	hf_wakeup_writer(hfp);
> +
> +	return true;
> +}
> +
>  bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
>  						const char *prefix,
>  						void *user_data,
> diff --git a/src/shared/hfp.h b/src/shared/hfp.h
> index 85037b1..5ee3017 100644
> --- a/src/shared/hfp.h
> +++ b/src/shared/hfp.h
> @@ -32,6 +32,8 @@ enum hfp_result {
>  	HFP_RESULT_NO_DIALTONE	= 6,
>  	HFP_RESULT_BUSY		= 7,
>  	HFP_RESULT_NO_ANSWER	= 8,
> +	HFP_RESULT_DELAYED	= 9,
> +	HFP_RESULT_BLACKLISTED	= 10,
>  };
>  
>  enum hfp_error {
> @@ -83,6 +85,10 @@ typedef void (*hfp_command_func_t)(const char *command, void *user_data);
>  
>  typedef void (*hfp_disconnect_func_t)(void *user_data);
>  
> +typedef void (*hfp_response_func_t)(const char *prefix,
> +							enum hfp_result result,
> +							void *user_data);
> +
>  struct hfp_gw;
>  struct hfp_hf;
>  
> @@ -146,3 +152,5 @@ bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
>  					const char *prefix, void *user_data,
>  					hfp_destroy_func_t destroy);
>  bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix);
> +bool hfp_hf_send_command(struct hfp_hf *hfp, hfp_response_func_t resp_cb,
> +				void *user_data, const char *format, ...);
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH v4 05/11] shared/hfp: Add HFP HF parser

[Szymon Janc]

Hi Łukasz,

On Friday 10 of October 2014 01:50:05 Lukasz Rymanowski wrote:
> This patch adds parser for AT responses and unsolicited results.
> ---
>  src/shared/hfp.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
> index b1cf08e..5179092 100644
> --- a/src/shared/hfp.c
> +++ b/src/shared/hfp.c
> @@ -868,6 +868,126 @@ static void destroy_event_handler(void *data)
>  	free(handler);
>  }
>  
> +static void hf_skip_whitespace(struct hfp_hf_result *result)
> +{
> +	while (result->data[result->offset] == ' ')
> +		result->offset++;
> +}
> +
> +static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
> +{
> +	struct event_handler *handler;
> +	const char *separators = ";:\0";
> +	struct hfp_hf_result result_data;
> +	char lookup_prefix[18];
> +	uint8_t pref_len = 0;
> +	const char *prefix;
> +	int i;
> +
> +	result_data.offset = 0;
> +	result_data.data = data;
> +
> +	hf_skip_whitespace(&result_data);
> +
> +	if (strlen(data + result_data.offset) < 2)
> +		return;
> +
> +	prefix = data + result_data.offset;
> +
> +	pref_len = strcspn(prefix, separators);
> +	if (pref_len > 17 || pref_len < 2)
> +		return;
> +
> +	for (i = 0; i < pref_len; i++)
> +		lookup_prefix[i] = toupper(prefix[i]);
> +
> +	lookup_prefix[pref_len] = '\0';
> +	result_data.offset += pref_len + 1;
> +
> +	handler = queue_find(hfp->event_handlers, match_handler_event_prefix,
> +								lookup_prefix);
> +	if (!handler)
> +		return;
> +
> +	handler->callback(&result_data, handler->user_data);
> +}
> +
> +static char *find_cr_lf(char *str, size_t len)
> +{
> +	char *ptr;
> +	int count;
> +	int offset;
> +
> +	offset = 0;
> +
> +	ptr = memchr(str, '\r', len);
> +	while (ptr) {
> +		/*
> +		 * Check if there is more data after '\r'. If so check for
> +		 * '\n'
> +		 */
> +		count = ptr-str;

Style: spaces around -

> +		if ((count < (int) (len - 1)) && *(ptr + 1) == '\n')
> +			return ptr;

If you make count size_t then this cast is not needed.

> +
> +		/* There is only '\r'? Let's try to find next one */
> +		offset += count + 1;
> +
> +		if (offset >= (int)len)

If you make offset size_t then this cast is not needed.
Also such casting should have space '(int) foo'.

> +			return NULL;
> +
> +		ptr = memchr(str + offset, '\r', len - offset);
> +	}
> +
> +	return NULL;
> +}
> +
> +static void hf_process_input(struct hfp_hf *hfp)
> +{
> +	char *str, *ptr;
> +	size_t len, count, offset;
> +
> +	str = ringbuf_peek(hfp->read_buf, 0, &len);
> +	if (!str)
> +		return;
> +
> +	offset = 0;
> +
> +	ptr = find_cr_lf(str, len);
> +	while (ptr) {
> +		count = ptr - (str + offset);

If you would adjust str pointer instead of using str + offset everywhere
then this code would be a bit simpler to follow.

Also this would not handle wrapped string correctly. Check how this is handled
in process_input().

> +		if (count == 0) {
> +			/* 2 is for <cr><lf> */
> +			offset += 2;
> +		} else {
> +			*ptr = '\0';
> +			hf_call_prefix_handler(hfp, str + offset);
> +			offset += count + 2;
> +		}
> +
> +		if (offset >= len)
> +			break;
> +
> +		ptr = find_cr_lf(str + offset, len - offset);
> +	}
> +
> +	ringbuf_drain(hfp->read_buf, offset < len ? offset : len);
> +}
> +
> +static bool hf_can_read_data(struct io *io, void *user_data)
> +{
> +	struct hfp_hf *hfp = user_data;
> +	ssize_t bytes_read;
> +
> +	bytes_read = ringbuf_read(hfp->read_buf, hfp->fd);
> +	if (bytes_read < 0)
> +		return false;
> +
> +	hf_process_input(hfp);
> +
> +	return true;
> +}
> +
>  struct hfp_hf *hfp_hf_new(int fd)
>  {
>  	struct hfp_hf *hfp;
> @@ -912,6 +1032,15 @@ struct hfp_hf *hfp_hf_new(int fd)
>  		return NULL;
>  	}
>  
> +	if (!io_set_read_handler(hfp->io, hf_can_read_data, hfp,
> +							read_watch_destroy)) {
> +		queue_destroy(hfp->event_handlers,
> +						destroy_event_handler);
> +		io_destroy(hfp->io);
> +		ringbuf_free(hfp->write_buf);
> +		ringbuf_free(hfp->read_buf);

You are missing free(hfp); return NULL; here.

> +	}
> +
>  	return hfp_hf_ref(hfp);
>  }
>  
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH v4 04/11] shared/hfp: Add register/unregister event for HFP HF

[Szymon Janc]

Hi Łukasz,

On Friday 10 of October 2014 01:50:04 Lukasz Rymanowski wrote:
> This patch adds API which allows to register/unregister for unsolicited
> responses.
> ---
>  src/shared/hfp.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/shared/hfp.h |   8 +++++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
> index b7855ed..b1cf08e 100644
> --- a/src/shared/hfp.c
> +++ b/src/shared/hfp.c
> @@ -70,6 +70,8 @@ struct hfp_hf {
>  	struct ringbuf *read_buf;
>  	struct ringbuf *write_buf;
>  
> +	struct queue *event_handlers;
> +
>  	hfp_debug_func_t debug_callback;
>  	hfp_destroy_func_t debug_destroy;
>  	void *debug_data;
> @@ -94,6 +96,18 @@ struct hfp_gw_result {
>  	unsigned int offset;
>  };
>  
> +struct hfp_hf_result {
> +	const char *data;
> +	unsigned int offset;
> +};
> +
> +struct event_handler {
> +	char *prefix;
> +	void *user_data;
> +	hfp_destroy_func_t destroy;
> +	hfp_hf_result_func_t callback;
> +};
> +
>  static void destroy_cmd_handler(void *data)
>  {
>  	struct cmd_handler *handler = data;
> @@ -828,6 +842,32 @@ bool hfp_gw_disconnect(struct hfp_gw *hfp)
>  	return io_shutdown(hfp->io);
>  }
>  
> +static bool match_handler_event_prefix(const void *a, const void *b)
> +{
> +	const struct event_handler *handler = a;
> +	const char *prefix = b;
> +
> +	if (strlen(handler->prefix) != strlen(prefix))
> +		return false;
> +
> +	if (memcmp(handler->prefix, prefix, strlen(prefix)))
> +		return false;

Why not just use strcmp() here?
I'm aware that this was based on gw code so you may also fix it there :)

> +
> +	return true;
> +}
> +
> +static void destroy_event_handler(void *data)
> +{
> +	struct event_handler *handler = data;
> +
> +	if (handler->destroy)
> +		handler->destroy(handler->user_data);
> +
> +	free(handler->prefix);
> +
> +	free(handler);
> +}
> +
>  struct hfp_hf *hfp_hf_new(int fd)
>  {
>  	struct hfp_hf *hfp;
> @@ -863,6 +903,15 @@ struct hfp_hf *hfp_hf_new(int fd)
>  		return NULL;
>  	}
>  
> +	hfp->event_handlers = queue_new();
> +	if (!hfp->event_handlers) {
> +		io_destroy(hfp->io);
> +		ringbuf_free(hfp->write_buf);
> +		ringbuf_free(hfp->read_buf);
> +		free(hfp);
> +		return NULL;
> +	}
> +
>  	return hfp_hf_ref(hfp);
>  }
>  
> @@ -902,6 +951,9 @@ void hfp_hf_unref(struct hfp_hf *hfp)
>  	ringbuf_free(hfp->write_buf);
>  	hfp->write_buf = NULL;
>  
> +	queue_destroy(hfp->event_handlers, destroy_event_handler);
> +	hfp->event_handlers = NULL;
> +
>  	if (!hfp->in_disconnect) {
>  		free(hfp);
>  		return;
> @@ -961,6 +1013,62 @@ bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close)
>  	return true;
>  }
>  
> +bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
> +						const char *prefix,
> +						void *user_data,
> +						hfp_destroy_func_t destroy)
> +{
> +	struct event_handler *handler;
> +
> +	if (!callback)
> +		return false;
> +
> +	handler = new0(struct event_handler, 1);
> +	if (!handler)
> +		return false;
> +
> +	handler->callback = callback;
> +	handler->user_data = user_data;
> +
> +	handler->prefix = strdup(prefix);
> +	if (!handler->prefix) {
> +		free(handler);
> +		return false;
> +	}
> +
> +	if (queue_find(hfp->event_handlers, match_handler_event_prefix,
> +							handler->prefix)) {
> +		destroy_event_handler(handler);
> +		return false;
> +	}
> +
> +	handler->destroy = destroy;
> +
> +	return queue_push_tail(hfp->event_handlers, handler);
> +}
> +
> +bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix)
> +{
> +	struct cmd_handler *handler;
> +	char *lookup_prefix;
> +
> +	lookup_prefix = strdup(prefix);
> +	if (!lookup_prefix)
> +		return false;

This strdup seems superfluous. If this is only due to to queue api being
non-const then I'd just cast it to (void *).

> +
> +	handler = queue_remove_if(hfp->event_handlers,
> +						match_handler_event_prefix,
> +						lookup_prefix);
> +	free(lookup_prefix);
> +
> +	if (!handler)
> +		return false;
> +
> +	destroy_event_handler(handler);
> +
> +	return true;
> +}
> +
>  static void hf_disconnect_watch_destroy(void *user_data)
>  {
>  	struct hfp_hf *hfp = user_data;
> diff --git a/src/shared/hfp.h b/src/shared/hfp.h
> index a9a169b..85037b1 100644
> --- a/src/shared/hfp.h
> +++ b/src/shared/hfp.h
> @@ -68,10 +68,14 @@ enum hfp_gw_cmd_type {
>  };
>  
>  struct hfp_gw_result;
> +struct hfp_hf_result;

As before, I'd keep hf stuff in single section.

>  
>  typedef void (*hfp_result_func_t)(struct hfp_gw_result *result,
>  				enum hfp_gw_cmd_type type, void *user_data);
>  
> +typedef void (*hfp_hf_result_func_t)(struct hfp_hf_result *result,
> +							void *user_data);
> +

Same here.

>  typedef void (*hfp_destroy_func_t)(void *user_data);
>  typedef void (*hfp_debug_func_t)(const char *str, void *user_data);
>  
> @@ -138,3 +142,7 @@ bool hfp_hf_set_disconnect_handler(struct hfp_hf *hfp,
>  					void *user_data,
>  					hfp_destroy_func_t destroy);
>  bool hfp_hf_disconnect(struct hfp_hf *hfp);
> +bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
> +					const char *prefix, void *user_data,
> +					hfp_destroy_func_t destroy);
> +bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix);
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH v4 01/11] shared/hfp: Add support for HFP HF

[Szymon Janc]

Hi Łukasz,

On Friday 10 of October 2014 01:50:01 Lukasz Rymanowski wrote:
> This patch add struct hfp_hf plus fuctions to create an instance ref and
> unref. This code based on existing hfp_gw
> ---
>  src/shared/hfp.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/shared/hfp.h |  6 ++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
> index efc981f..dbd049a 100644
> --- a/src/shared/hfp.c
> +++ b/src/shared/hfp.c
> @@ -62,6 +62,18 @@ struct hfp_gw {
>  	bool destroyed;
>  };
>  
> +struct hfp_hf {
> +	int ref_count;
> +	int fd;
> +	bool close_on_unref;
> +	struct io *io;
> +	struct ringbuf *read_buf;
> +	struct ringbuf *write_buf;
> +
> +	bool in_disconnect;
> +	bool destroyed;
> +};
> +
>  struct cmd_handler {
>  	char *prefix;
>  	void *user_data;
> @@ -807,3 +819,83 @@ bool hfp_gw_disconnect(struct hfp_gw *hfp)
>  
>  	return io_shutdown(hfp->io);
>  }
> +
> +struct hfp_hf *hfp_hf_new(int fd)
> +{
> +	struct hfp_hf *hfp;
> +
> +	if (fd < 0)
> +		return NULL;
> +
> +	hfp = new0(struct hfp_hf, 1);
> +	if (!hfp)
> +		return NULL;
> +
> +	hfp->fd = fd;
> +	hfp->close_on_unref = false;
> +
> +	hfp->read_buf = ringbuf_new(4096);
> +	if (!hfp->read_buf) {
> +		free(hfp);
> +		return NULL;
> +	}
> +
> +	hfp->write_buf = ringbuf_new(4096);
> +	if (!hfp->write_buf) {
> +		ringbuf_free(hfp->read_buf);
> +		free(hfp);
> +		return NULL;
> +	}
> +
> +	hfp->io = io_new(fd);
> +	if (!hfp->io) {
> +		ringbuf_free(hfp->write_buf);
> +		ringbuf_free(hfp->read_buf);
> +		free(hfp);
> +		return NULL;
> +	}
> +
> +	return hfp_hf_ref(hfp);
> +}
> +
> +struct hfp_hf *hfp_hf_ref(struct hfp_hf *hfp)
> +{
> +	if (!hfp)
> +		return NULL;
> +
> +	__sync_fetch_and_add(&hfp->ref_count, 1);
> +
> +	return hfp;
> +}
> +
> +void hfp_hf_unref(struct hfp_hf *hfp)
> +{
> +	if (!hfp)
> +		return;
> +
> +	if (__sync_sub_and_fetch(&hfp->ref_count, 1))
> +		return;
> +
> +	io_set_write_handler(hfp->io, NULL, NULL, NULL);
> +	io_set_read_handler(hfp->io, NULL, NULL, NULL);
> +	io_set_disconnect_handler(hfp->io, NULL, NULL, NULL);
> +
> +	io_destroy(hfp->io);
> +	hfp->io = NULL;
> +
> +	if (hfp->close_on_unref)
> +		close(hfp->fd);
> +
> +	ringbuf_free(hfp->read_buf);
> +	hfp->read_buf = NULL;
> +
> +	ringbuf_free(hfp->write_buf);
> +	hfp->write_buf = NULL;
> +
> +	if (!hfp->in_disconnect) {
> +		free(hfp);
> +		return;
> +	}
> +
> +	hfp->destroyed = true;
> +}
> diff --git a/src/shared/hfp.h b/src/shared/hfp.h
> index 743db65..50d9c4b 100644
> --- a/src/shared/hfp.h
> +++ b/src/shared/hfp.h
> @@ -76,9 +76,11 @@ typedef void (*hfp_destroy_func_t)(void *user_data);
>  typedef void (*hfp_debug_func_t)(const char *str, void *user_data);
>  
>  typedef void (*hfp_command_func_t)(const char *command, void *user_data);
> +

Unrelated.

>  typedef void (*hfp_disconnect_func_t)(void *user_data);
>  
>  struct hfp_gw;
> +struct hfp_hf;

I'd prefer if we have all hfp_hf stuff in same section.

>  
>  struct hfp_gw *hfp_gw_new(int fd);
>  
> @@ -124,3 +126,7 @@ bool hfp_gw_result_get_string(struct hfp_gw_result *result, char *buf,
>  bool hfp_gw_result_get_unquoted_string(struct hfp_gw_result *result, char *buf,
>  								uint8_t len);
>  bool hfp_gw_result_has_next(struct hfp_gw_result *result);
> +
> +struct hfp_hf *hfp_hf_new(int fd);
> +struct hfp_hf *hfp_hf_ref(struct hfp_hf *hfp);
> +void hfp_hf_unref(struct hfp_hf *hfp);
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH v4 04/11] shared/hfp: Add register/unregister event for HFP HF

[Lukasz Rymanowski]

Hi Szymon,

On 22 October 2014 13:00, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Friday 10 of October 2014 01:50:04 Lukasz Rymanowski wrote:
>> This patch adds API which allows to register/unregister for unsolicited
>> responses.
>> ---
>>  src/shared/hfp.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/shared/hfp.h |   8 +++++
>>  2 files changed, 116 insertions(+)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index b7855ed..b1cf08e 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -70,6 +70,8 @@ struct hfp_hf {
>>       struct ringbuf *read_buf;
>>       struct ringbuf *write_buf;
>>
>> +     struct queue *event_handlers;
>> +
>>       hfp_debug_func_t debug_callback;
>>       hfp_destroy_func_t debug_destroy;
>>       void *debug_data;
>> @@ -94,6 +96,18 @@ struct hfp_gw_result {
>>       unsigned int offset;
>>  };
>>
>> +struct hfp_hf_result {
>> +     const char *data;
>> +     unsigned int offset;
>> +};
>> +
>> +struct event_handler {
>> +     char *prefix;
>> +     void *user_data;
>> +     hfp_destroy_func_t destroy;
>> +     hfp_hf_result_func_t callback;
>> +};
>> +
>>  static void destroy_cmd_handler(void *data)
>>  {
>>       struct cmd_handler *handler = data;
>> @@ -828,6 +842,32 @@ bool hfp_gw_disconnect(struct hfp_gw *hfp)
>>       return io_shutdown(hfp->io);
>>  }
>>
>> +static bool match_handler_event_prefix(const void *a, const void *b)
>> +{
>> +     const struct event_handler *handler = a;
>> +     const char *prefix = b;
>> +
>> +     if (strlen(handler->prefix) != strlen(prefix))
>> +             return false;
>> +
>> +     if (memcmp(handler->prefix, prefix, strlen(prefix)))
>> +             return false;
>
> Why not just use strcmp() here?
> I'm aware that this was based on gw code so you may also fix it there :)

Copy paste issue. But that's correct. Will send also patch for base code.
>
>> +
>> +     return true;
>> +}
>> +
>> +static void destroy_event_handler(void *data)
>> +{
>> +     struct event_handler *handler = data;
>> +
>> +     if (handler->destroy)
>> +             handler->destroy(handler->user_data);
>> +
>> +     free(handler->prefix);
>> +
>> +     free(handler);
>> +}
>> +
>>  struct hfp_hf *hfp_hf_new(int fd)
>>  {
>>       struct hfp_hf *hfp;
>> @@ -863,6 +903,15 @@ struct hfp_hf *hfp_hf_new(int fd)
>>               return NULL;
>>       }
>>
>> +     hfp->event_handlers = queue_new();
>> +     if (!hfp->event_handlers) {
>> +             io_destroy(hfp->io);
>> +             ringbuf_free(hfp->write_buf);
>> +             ringbuf_free(hfp->read_buf);
>> +             free(hfp);
>> +             return NULL;
>> +     }
>> +
>>       return hfp_hf_ref(hfp);
>>  }
>>
>> @@ -902,6 +951,9 @@ void hfp_hf_unref(struct hfp_hf *hfp)
>>       ringbuf_free(hfp->write_buf);
>>       hfp->write_buf = NULL;
>>
>> +     queue_destroy(hfp->event_handlers, destroy_event_handler);
>> +     hfp->event_handlers = NULL;
>> +
>>       if (!hfp->in_disconnect) {
>>               free(hfp);
>>               return;
>> @@ -961,6 +1013,62 @@ bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close)
>>       return true;
>>  }
>>
>> +bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
>> +                                             const char *prefix,
>> +                                             void *user_data,
>> +                                             hfp_destroy_func_t destroy)
>> +{
>> +     struct event_handler *handler;
>> +
>> +     if (!callback)
>> +             return false;
>> +
>> +     handler = new0(struct event_handler, 1);
>> +     if (!handler)
>> +             return false;
>> +
>> +     handler->callback = callback;
>> +     handler->user_data = user_data;
>> +
>> +     handler->prefix = strdup(prefix);
>> +     if (!handler->prefix) {
>> +             free(handler);
>> +             return false;
>> +     }
>> +
>> +     if (queue_find(hfp->event_handlers, match_handler_event_prefix,
>> +                                                     handler->prefix)) {
>> +             destroy_event_handler(handler);
>> +             return false;
>> +     }
>> +
>> +     handler->destroy = destroy;
>> +
>> +     return queue_push_tail(hfp->event_handlers, handler);
>> +}
>> +
>> +bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix)
>> +{
>> +     struct cmd_handler *handler;
>> +     char *lookup_prefix;
>> +
>> +     lookup_prefix = strdup(prefix);
>> +     if (!lookup_prefix)
>> +             return false;
>
> This strdup seems superfluous. If this is only due to to queue api being
> non-const then I'd just cast it to (void *).

:) ditto.

>
>> +
>> +     handler = queue_remove_if(hfp->event_handlers,
>> +                                             match_handler_event_prefix,
>> +                                             lookup_prefix);
>> +     free(lookup_prefix);
>> +
>> +     if (!handler)
>> +             return false;
>> +
>> +     destroy_event_handler(handler);
>> +
>> +     return true;
>> +}
>> +
>>  static void hf_disconnect_watch_destroy(void *user_data)
>>  {
>>       struct hfp_hf *hfp = user_data;
>> diff --git a/src/shared/hfp.h b/src/shared/hfp.h
>> index a9a169b..85037b1 100644
>> --- a/src/shared/hfp.h
>> +++ b/src/shared/hfp.h
>> @@ -68,10 +68,14 @@ enum hfp_gw_cmd_type {
>>  };
>>
>>  struct hfp_gw_result;
>> +struct hfp_hf_result;
>
> As before, I'd keep hf stuff in single section.

This and the one below will finally goes out as it will be merget into
hfp_context. I think there is no sense to change that here now.

>
>>
>>  typedef void (*hfp_result_func_t)(struct hfp_gw_result *result,
>>                               enum hfp_gw_cmd_type type, void *user_data);
>>
>> +typedef void (*hfp_hf_result_func_t)(struct hfp_hf_result *result,
>> +                                                     void *user_data);
>> +
>
> Same here.

ditto.

>
>>  typedef void (*hfp_destroy_func_t)(void *user_data);
>>  typedef void (*hfp_debug_func_t)(const char *str, void *user_data);
>>
>> @@ -138,3 +142,7 @@ bool hfp_hf_set_disconnect_handler(struct hfp_hf *hfp,
>>                                       void *user_data,
>>                                       hfp_destroy_func_t destroy);
>>  bool hfp_hf_disconnect(struct hfp_hf *hfp);
>> +bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
>> +                                     const char *prefix, void *user_data,
>> +                                     hfp_destroy_func_t destroy);
>> +bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix);
>>
>
> --
> Best regards,
> Szymon Janc

Re: [PATCH v4 05/11] shared/hfp: Add HFP HF parser

[Lukasz Rymanowski]

Hi Szymon,

On 22 October 2014 13:00, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Friday 10 of October 2014 01:50:05 Lukasz Rymanowski wrote:
>> This patch adds parser for AT responses and unsolicited results.
>> ---
>>  src/shared/hfp.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 129 insertions(+)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index b1cf08e..5179092 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -868,6 +868,126 @@ static void destroy_event_handler(void *data)
>>       free(handler);
>>  }
>>
>> +static void hf_skip_whitespace(struct hfp_hf_result *result)
>> +{
>> +     while (result->data[result->offset] == ' ')
>> +             result->offset++;
>> +}
>> +
>> +static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
>> +{
>> +     struct event_handler *handler;
>> +     const char *separators = ";:\0";
>> +     struct hfp_hf_result result_data;
>> +     char lookup_prefix[18];
>> +     uint8_t pref_len = 0;
>> +     const char *prefix;
>> +     int i;
>> +
>> +     result_data.offset = 0;
>> +     result_data.data = data;
>> +
>> +     hf_skip_whitespace(&result_data);
>> +
>> +     if (strlen(data + result_data.offset) < 2)
>> +             return;
>> +
>> +     prefix = data + result_data.offset;
>> +
>> +     pref_len = strcspn(prefix, separators);
>> +     if (pref_len > 17 || pref_len < 2)
>> +             return;
>> +
>> +     for (i = 0; i < pref_len; i++)
>> +             lookup_prefix[i] = toupper(prefix[i]);
>> +
>> +     lookup_prefix[pref_len] = '\0';
>> +     result_data.offset += pref_len + 1;
>> +
>> +     handler = queue_find(hfp->event_handlers, match_handler_event_prefix,
>> +                                                             lookup_prefix);
>> +     if (!handler)
>> +             return;
>> +
>> +     handler->callback(&result_data, handler->user_data);
>> +}
>> +
>> +static char *find_cr_lf(char *str, size_t len)
>> +{
>> +     char *ptr;
>> +     int count;
>> +     int offset;
>> +
>> +     offset = 0;
>> +
>> +     ptr = memchr(str, '\r', len);
>> +     while (ptr) {
>> +             /*
>> +              * Check if there is more data after '\r'. If so check for
>> +              * '\n'
>> +              */
>> +             count = ptr-str;
>
> Style: spaces around -

>
>> +             if ((count < (int) (len - 1)) && *(ptr + 1) == '\n')
>> +                     return ptr;
>
> If you make count size_t then this cast is not needed.
>
>> +
>> +             /* There is only '\r'? Let's try to find next one */
>> +             offset += count + 1;
>> +
>> +             if (offset >= (int)len)
>
> If you make offset size_t then this cast is not needed.
> Also such casting should have space '(int) foo'.
>
>> +                     return NULL;
>> +
>> +             ptr = memchr(str + offset, '\r', len - offset);
>> +     }
>> +
>> +     return NULL;
>> +}
>> +
>> +static void hf_process_input(struct hfp_hf *hfp)
>> +{
>> +     char *str, *ptr;
>> +     size_t len, count, offset;
>> +
>> +     str = ringbuf_peek(hfp->read_buf, 0, &len);
>> +     if (!str)
>> +             return;
>> +
>> +     offset = 0;
>> +
>> +     ptr = find_cr_lf(str, len);
>> +     while (ptr) {
>> +             count = ptr - (str + offset);
>
> If you would adjust str pointer instead of using str + offset everywhere
> then this code would be a bit simpler to follow.
>
see below

> Also this would not handle wrapped string correctly. Check how this is handled
> in process_input().

Missed that. Will fix, also will fix that code as asprintf should not
be use. str is not a string in case buffer is wrapped.
Also will need to keep offset so I can concatenate str and str2 which
will come from the begging of ringbuf
>
>> +             if (count == 0) {
>> +                     /* 2 is for <cr><lf> */
>> +                     offset += 2;
>> +             } else {
>> +                     *ptr = '\0';
>> +                     hf_call_prefix_handler(hfp, str + offset);
>> +                     offset += count + 2;
>> +             }
>> +
>> +             if (offset >= len)
>> +                     break;
>> +
>> +             ptr = find_cr_lf(str + offset, len - offset);
>> +     }
>> +
>> +     ringbuf_drain(hfp->read_buf, offset < len ? offset : len);
>> +}
>> +
>> +static bool hf_can_read_data(struct io *io, void *user_data)
>> +{
>> +     struct hfp_hf *hfp = user_data;
>> +     ssize_t bytes_read;
>> +
>> +     bytes_read = ringbuf_read(hfp->read_buf, hfp->fd);
>> +     if (bytes_read < 0)
>> +             return false;
>> +
>> +     hf_process_input(hfp);
>> +
>> +     return true;
>> +}
>> +
>>  struct hfp_hf *hfp_hf_new(int fd)
>>  {
>>       struct hfp_hf *hfp;
>> @@ -912,6 +1032,15 @@ struct hfp_hf *hfp_hf_new(int fd)
>>               return NULL;
>>       }
>>
>> +     if (!io_set_read_handler(hfp->io, hf_can_read_data, hfp,
>> +                                                     read_watch_destroy)) {
>> +             queue_destroy(hfp->event_handlers,
>> +                                             destroy_event_handler);
>> +             io_destroy(hfp->io);
>> +             ringbuf_free(hfp->write_buf);
>> +             ringbuf_free(hfp->read_buf);
>
> You are missing free(hfp); return NULL; here.

ah, some rebase issue. thanks

>
>> +     }
>> +
>>       return hfp_hf_ref(hfp);
>>  }
>>
>>
>
> --
> Best regards,
> Szymon Janc

\Łukasz

Re: [PATCH v4 01/11] shared/hfp: Add support for HFP HF

[Lukasz Rymanowski]

Hi Szymon,

On 22 October 2014 13:00, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Friday 10 of October 2014 01:50:01 Lukasz Rymanowski wrote:
>> This patch add struct hfp_hf plus fuctions to create an instance ref and
>> unref. This code based on existing hfp_gw
>> ---
>>  src/shared/hfp.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/shared/hfp.h |  6 ++++
>>  2 files changed, 98 insertions(+)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index efc981f..dbd049a 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -62,6 +62,18 @@ struct hfp_gw {
>>       bool destroyed;
>>  };
>>
>> +struct hfp_hf {
>> +     int ref_count;
>> +     int fd;
>> +     bool close_on_unref;
>> +     struct io *io;
>> +     struct ringbuf *read_buf;
>> +     struct ringbuf *write_buf;
>> +
>> +     bool in_disconnect;
>> +     bool destroyed;
>> +};
>> +
>>  struct cmd_handler {
>>       char *prefix;
>>       void *user_data;
>> @@ -807,3 +819,83 @@ bool hfp_gw_disconnect(struct hfp_gw *hfp)
>>
>>       return io_shutdown(hfp->io);
>>  }
>> +
>> +struct hfp_hf *hfp_hf_new(int fd)
>> +{
>> +     struct hfp_hf *hfp;
>> +
>> +     if (fd < 0)
>> +             return NULL;
>> +
>> +     hfp = new0(struct hfp_hf, 1);
>> +     if (!hfp)
>> +             return NULL;
>> +
>> +     hfp->fd = fd;
>> +     hfp->close_on_unref = false;
>> +
>> +     hfp->read_buf = ringbuf_new(4096);
>> +     if (!hfp->read_buf) {
>> +             free(hfp);
>> +             return NULL;
>> +     }
>> +
>> +     hfp->write_buf = ringbuf_new(4096);
>> +     if (!hfp->write_buf) {
>> +             ringbuf_free(hfp->read_buf);
>> +             free(hfp);
>> +             return NULL;
>> +     }
>> +
>> +     hfp->io = io_new(fd);
>> +     if (!hfp->io) {
>> +             ringbuf_free(hfp->write_buf);
>> +             ringbuf_free(hfp->read_buf);
>> +             free(hfp);
>> +             return NULL;
>> +     }
>> +
>> +     return hfp_hf_ref(hfp);
>> +}
>> +
>> +struct hfp_hf *hfp_hf_ref(struct hfp_hf *hfp)
>> +{
>> +     if (!hfp)
>> +             return NULL;
>> +
>> +     __sync_fetch_and_add(&hfp->ref_count, 1);
>> +
>> +     return hfp;
>> +}
>> +
>> +void hfp_hf_unref(struct hfp_hf *hfp)
>> +{
>> +     if (!hfp)
>> +             return;
>> +
>> +     if (__sync_sub_and_fetch(&hfp->ref_count, 1))
>> +             return;
>> +
>> +     io_set_write_handler(hfp->io, NULL, NULL, NULL);
>> +     io_set_read_handler(hfp->io, NULL, NULL, NULL);
>> +     io_set_disconnect_handler(hfp->io, NULL, NULL, NULL);
>> +
>> +     io_destroy(hfp->io);
>> +     hfp->io = NULL;
>> +
>> +     if (hfp->close_on_unref)
>> +             close(hfp->fd);
>> +
>> +     ringbuf_free(hfp->read_buf);
>> +     hfp->read_buf = NULL;
>> +
>> +     ringbuf_free(hfp->write_buf);
>> +     hfp->write_buf = NULL;
>> +
>> +     if (!hfp->in_disconnect) {
>> +             free(hfp);
>> +             return;
>> +     }
>> +
>> +     hfp->destroyed = true;
>> +}
>> diff --git a/src/shared/hfp.h b/src/shared/hfp.h
>> index 743db65..50d9c4b 100644
>> --- a/src/shared/hfp.h
>> +++ b/src/shared/hfp.h
>> @@ -76,9 +76,11 @@ typedef void (*hfp_destroy_func_t)(void *user_data);
>>  typedef void (*hfp_debug_func_t)(const char *str, void *user_data);
>>
>>  typedef void (*hfp_command_func_t)(const char *command, void *user_data);
>> +
>
> Unrelated.

True.

>
>>  typedef void (*hfp_disconnect_func_t)(void *user_data);
>>
>>  struct hfp_gw;
>> +struct hfp_hf;
>
> I'd prefer if we have all hfp_hf stuff in same section.

OK

>
>>
>>  struct hfp_gw *hfp_gw_new(int fd);
>>
>> @@ -124,3 +126,7 @@ bool hfp_gw_result_get_string(struct hfp_gw_result *result, char *buf,
>>  bool hfp_gw_result_get_unquoted_string(struct hfp_gw_result *result, char *buf,
>>                                                               uint8_t len);
>>  bool hfp_gw_result_has_next(struct hfp_gw_result *result);
>> +
>> +struct hfp_hf *hfp_hf_new(int fd);
>> +struct hfp_hf *hfp_hf_ref(struct hfp_hf *hfp);
>> +void hfp_hf_unref(struct hfp_hf *hfp);
>>
>
> --
> Best regards,
> Szymon Janc

\Łukasz

Re: [PATCH v4 06/11] shared/hfp: Add send AT command API for HFP HF

[Lukasz Rymanowski]

Hi Szymon,

On 22 October 2014 13:00, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Friday 10 of October 2014 01:50:06 Lukasz Rymanowski wrote:
>> This patch adds handling send and response of AT command.
>> Note that we always wait for AT command response before sending next
>> command, however user can fill hfp_hf with more than one command.
>> All the commands are queued and send one by one.
>> ---
>>  src/shared/hfp.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/shared/hfp.h |   8 +++
>>  2 files changed, 183 insertions(+)
>>
>> diff --git a/src/shared/hfp.c b/src/shared/hfp.c
>> index 5179092..8bebe97 100644
>> --- a/src/shared/hfp.c
>> +++ b/src/shared/hfp.c
>> @@ -70,6 +70,9 @@ struct hfp_hf {
>>       struct ringbuf *read_buf;
>>       struct ringbuf *write_buf;
>>
>> +     bool writer_active;
>> +     struct queue *cmd_queue;
>> +
>>       struct queue *event_handlers;
>>
>>       hfp_debug_func_t debug_callback;
>> @@ -101,6 +104,14 @@ struct hfp_hf_result {
>>       unsigned int offset;
>>  };
>>
>> +struct cmd_response {
>> +     char *prefix;
>> +     hfp_response_func_t resp_cb;
>> +     struct hfp_hf_result *response;
>> +     char *resp_data;
>> +     void *user_data;
>> +};
>> +
>>  struct event_handler {
>>       char *prefix;
>>       void *user_data;
>> @@ -868,17 +879,103 @@ static void destroy_event_handler(void *data)
>>       free(handler);
>>  }
>>
>> +static bool hf_can_write_data(struct io *io, void *user_data)
>> +{
>> +     struct hfp_hf *hfp = user_data;
>> +     ssize_t bytes_written;
>> +
>> +     bytes_written = ringbuf_write(hfp->write_buf, hfp->fd);
>> +     if (bytes_written < 0)
>> +             return false;
>> +
>> +     if (ringbuf_len(hfp->write_buf) > 0)
>> +             return true;
>> +
>> +     return false;
>> +}
>> +
>> +static void hf_write_watch_destroy(void *user_data)
>> +{
>> +     struct hfp_hf *hfp = user_data;
>> +
>> +     hfp->writer_active = false;
>> +}
>> +
>>  static void hf_skip_whitespace(struct hfp_hf_result *result)
>>  {
>>       while (result->data[result->offset] == ' ')
>>               result->offset++;
>>  }
>>
>> +static bool is_response(const char *prefix, enum hfp_result *result)
>> +{
>> +     if (strcmp(prefix, "OK") == 0) {
>> +             *result = HFP_RESULT_OK;
>> +             return true;
>> +     }
>> +
>> +     if (strcmp(prefix, "ERROR") == 0) {
>> +             *result = HFP_RESULT_ERROR;
>> +             return true;
>> +     }
>> +
>> +     if (strcmp(prefix, "NO CARRIER") == 0) {
>> +             *result = HFP_RESULT_NO_CARRIER;
>> +             return true;
>> +     }
>> +
>> +     if (strcmp(prefix, "CONNECT") == 0) {
>
> Shouldn't this be "BUSY"?
>
>> +             *result = HFP_RESULT_CONNECT;
>
> And this enum value looks bogus to me.
> I couldn't find it in HFP nor HSP spec. Probably leftover from AT spec.
> I'd just handle what is defined in HFP spec here.

This comes from AT spec. I based on hfp_result enum but indeed this is
not needed. Will fix that and hfp_result enum. Agree that we need only
HFP/HSP related result here.
>
>> +             return true;
>> +     }
>> +
>> +     if (strcmp(prefix, "NO ANSWER") == 0) {
>> +             *result = HFP_RESULT_NO_ANSWER;
>> +             return true;
>> +     }
>> +
>> +     if (strcmp(prefix, "DELAYED") == 0) {
>> +             *result = HFP_RESULT_DELAYED;
>> +             return true;
>> +     }
>> +
>> +     if (strcmp(prefix, "BLACKLISTED") == 0) {
>> +             *result = HFP_RESULT_BLACKLISTED;
>> +             return true;
>> +     }
>> +
>> +     return false;
>> +}
>> +
>> +static void hf_wakeup_writer(struct hfp_hf *hfp)
>> +{
>> +     if (hfp->writer_active)
>> +             return;
>> +
>> +     if (!ringbuf_len(hfp->write_buf))
>> +             return;
>> +
>> +     if (!io_set_write_handler(hfp->io, hf_can_write_data,
>> +                                     hfp, hf_write_watch_destroy))
>> +             return;
>> +
>> +     hfp->writer_active = true;
>> +}
>> +
>> +static void destroy_cmd_response(void *data)
>> +{
>> +     struct cmd_response *cmd = data;
>> +
>> +     free(cmd->prefix);
>> +     free(cmd);
>> +}
>> +
>>  static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
>>  {
>>       struct event_handler *handler;
>>       const char *separators = ";:\0";
>>       struct hfp_hf_result result_data;
>> +     enum hfp_result result;
>>       char lookup_prefix[18];
>>       uint8_t pref_len = 0;
>>       const char *prefix;
>> @@ -904,6 +1001,22 @@ static void hf_call_prefix_handler(struct hfp_hf *hfp, const char *data)
>>       lookup_prefix[pref_len] = '\0';
>>       result_data.offset += pref_len + 1;
>>
>> +     if (is_response(lookup_prefix, &result)) {
>> +             struct cmd_response *cmd;
>> +
>> +             cmd = queue_peek_head(hfp->cmd_queue);
>> +             if (!cmd)
>> +                     return;
>> +
>> +             cmd->resp_cb(cmd->prefix, result, cmd->user_data);
>> +
>> +             queue_remove(hfp->cmd_queue, cmd);
>> +             destroy_cmd_response(cmd);
>> +
>> +             hf_wakeup_writer(hfp);
>> +             return;
>> +     }
>> +
>>       handler = queue_find(hfp->event_handlers, match_handler_event_prefix,
>>                                                               lookup_prefix);
>>       if (!handler)
>> @@ -1032,6 +1145,18 @@ struct hfp_hf *hfp_hf_new(int fd)
>>               return NULL;
>>       }
>>
>> +     hfp->cmd_queue = queue_new();
>> +     if (!hfp->cmd_queue) {
>> +             io_destroy(hfp->io);
>> +             ringbuf_free(hfp->write_buf);
>> +             ringbuf_free(hfp->read_buf);
>> +             queue_destroy(hfp->event_handlers, NULL);
>> +             free(hfp);
>> +             return NULL;
>> +     }
>> +
>> +     hfp->writer_active = false;
>> +
>>       if (!io_set_read_handler(hfp->io, hf_can_read_data, hfp,
>>                                                       read_watch_destroy)) {
>>               queue_destroy(hfp->event_handlers,
>> @@ -1083,6 +1208,9 @@ void hfp_hf_unref(struct hfp_hf *hfp)
>>       queue_destroy(hfp->event_handlers, destroy_event_handler);
>>       hfp->event_handlers = NULL;
>>
>> +     queue_destroy(hfp->cmd_queue, destroy_cmd_response);
>> +     hfp->cmd_queue = NULL;
>> +
>>       if (!hfp->in_disconnect) {
>>               free(hfp);
>>               return;
>> @@ -1142,6 +1270,53 @@ bool hfp_hf_set_close_on_unref(struct hfp_hf *hfp, bool do_close)
>>       return true;
>>  }
>>
>> +bool hfp_hf_send_command(struct hfp_hf *hfp, hfp_response_func_t resp_cb,
>> +                             void *user_data, const char *format, ...)
>> +{
>> +     va_list ap;
>> +     char *fmt;
>> +     int len;
>> +     const char *separators = ";?=\0";
>> +     uint8_t prefix_len;
>> +     struct cmd_response *cmd;
>> +
>> +     if (!hfp || !format || !resp_cb)
>> +             return false;
>> +
>> +     if (asprintf(&fmt, "%s\r", format) < 0)
>> +             return false;
>> +
>> +     cmd = new0(struct cmd_response, 1);
>> +     if (!cmd)
>> +             return false;
>> +
>> +     va_start(ap, format);
>> +     len = ringbuf_vprintf(hfp->write_buf, fmt, ap);
>> +     va_end(ap);
>> +
>> +     free(fmt);
>> +
>> +     if (len < 0) {
>> +             free(cmd);
>> +             return false;
>> +     }
>> +
>> +     prefix_len = strcspn(format, separators);
>
> I'd explore possibility of passing prefix as separate argument to the
> function to avoid need of this extra parsing. Also do we really need this
> prefix at all? We should not have more than one pending AT command anyway.

Well this is some leftover from my previous patches where I based on
it (had single function for command complete where I check that and
move with SLC connection setup) Now It is not needed in so will
basically remove it.

>
>> +     cmd->prefix = strndup(format, prefix_len);
>> +     cmd->resp_cb = resp_cb;
>> +     cmd->user_data = user_data;
>> +
>> +     if (!queue_push_tail(hfp->cmd_queue, cmd)) {
>> +             ringbuf_drain(hfp->write_buf, len);
>> +             free(cmd);
>> +             return false;
>> +     }
>> +
>> +     hf_wakeup_writer(hfp);
>> +
>> +     return true;
>> +}
>> +
>>  bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
>>                                               const char *prefix,
>>                                               void *user_data,
>> diff --git a/src/shared/hfp.h b/src/shared/hfp.h
>> index 85037b1..5ee3017 100644
>> --- a/src/shared/hfp.h
>> +++ b/src/shared/hfp.h
>> @@ -32,6 +32,8 @@ enum hfp_result {
>>       HFP_RESULT_NO_DIALTONE  = 6,
>>       HFP_RESULT_BUSY         = 7,
>>       HFP_RESULT_NO_ANSWER    = 8,
>> +     HFP_RESULT_DELAYED      = 9,
>> +     HFP_RESULT_BLACKLISTED  = 10,
>>  };
>>
>>  enum hfp_error {
>> @@ -83,6 +85,10 @@ typedef void (*hfp_command_func_t)(const char *command, void *user_data);
>>
>>  typedef void (*hfp_disconnect_func_t)(void *user_data);
>>
>> +typedef void (*hfp_response_func_t)(const char *prefix,
>> +                                                     enum hfp_result result,
>> +                                                     void *user_data);
>> +
>>  struct hfp_gw;
>>  struct hfp_hf;
>>
>> @@ -146,3 +152,5 @@ bool hfp_hf_register(struct hfp_hf *hfp, hfp_hf_result_func_t callback,
>>                                       const char *prefix, void *user_data,
>>                                       hfp_destroy_func_t destroy);
>>  bool hfp_hf_unregister(struct hfp_hf *hfp, const char *prefix);
>> +bool hfp_hf_send_command(struct hfp_hf *hfp, hfp_response_func_t resp_cb,
>> +                             void *user_data, const char *format, ...);
>>
>
> --
> Best regards,
> Szymon Janc

BR
Lukasz

Re: [PATCH v4 08/11] unit/test-hfp: Add init test for HFP HF

[Lukasz Rymanowski]

Hi Szymon,

On 22 October 2014 13:00, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Friday 10 of October 2014 01:50:08 Lukasz Rymanowski wrote:
>> This patch adds basic infrastruction for HFP HF test plus
>> init test.
>>
>> It also moves send_pdu function in the file so it can be used by
>> test_hf_handler
>> ---
>>  unit/test-hfp.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 84 insertions(+), 18 deletions(-)
>>
>> diff --git a/unit/test-hfp.c b/unit/test-hfp.c
>> index 4b3473b..274ee55 100644
>> --- a/unit/test-hfp.c
>> +++ b/unit/test-hfp.c
>> @@ -36,6 +36,7 @@ struct context {
>>       int fd_server;
>>       int fd_client;
>>       struct hfp_gw *hfp;
>> +     struct hfp_hf *hfp_hf;
>>       const struct test_data *data;
>>       unsigned int pdu_offset;
>>  };
>> @@ -52,6 +53,8 @@ struct test_data {
>>       char *test_name;
>>       struct test_pdu *pdu_list;
>>       hfp_result_func_t result_func;
>> +     hfp_response_func_t response_func;
>> +     hfp_hf_result_func_t hf_result_func;
>>       GIOFunc test_handler;
>>  };
>>
>> @@ -99,6 +102,22 @@ struct test_data {
>>               data.test_handler = test_handler;                       \
>>       } while (0)
>>
>> +#define define_hf_test(name, function, result_func, response_function,       \
>> +                                                             args...)\
>> +     do {                                                            \
>> +             const struct test_pdu pdus[] = {                        \
>> +                     args, { }                                       \
>> +             };                                                      \
>> +             static struct test_data data;                           \
>> +             data.test_name = g_strdup(name);                        \
>> +             data.pdu_list = g_malloc(sizeof(pdus));                 \
>> +             data.hf_result_func = result_func;                      \
>> +             data.response_func = response_function;                 \
>> +             memcpy(data.pdu_list, pdus, sizeof(pdus));              \
>> +             g_test_add_data_func(name, &data, function);            \
>> +             data.test_handler = test_hf_handler;                    \
>> +     } while (0)
>> +
>>  static void context_quit(struct context *context)
>>  {
>>       g_main_loop_quit(context->main_loop);
>> @@ -128,6 +147,52 @@ static gboolean test_handler(GIOChannel *channel, GIOCondition cond,
>>       return FALSE;
>>  }
>>
>> +static gboolean send_pdu(gpointer user_data)
>> +{
>> +     struct context *context = user_data;
>> +     const struct test_pdu *pdu;
>> +     ssize_t len;
>> +
>> +     pdu = &context->data->pdu_list[context->pdu_offset++];
>> +
>> +     if (pdu && !pdu->valid)
>> +             return FALSE;
>> +
>> +     len = write(context->fd_server, pdu->data, pdu->size);
>> +     g_assert_cmpint(len, ==, pdu->size);
>> +
>> +     pdu = &context->data->pdu_list[context->pdu_offset];
>> +     if (pdu->fragmented)
>> +             g_idle_add(send_pdu, context);
>> +
>> +     return FALSE;
>> +}
>> +
>> +static gboolean test_hf_handler(GIOChannel *channel, GIOCondition cond,
>> +                                                     gpointer user_data)
>> +{
>> +     struct context *context = user_data;
>> +     gchar buf[60];
>> +     gsize bytes_read;
>> +     GError *error = NULL;
>> +
>> +     if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
>> +             goto done;
>> +
>> +     /* dummy read */
>> +     g_io_channel_read_chars(channel, buf, 60, &bytes_read, &error);
>> +
>> +     send_pdu(context);
>> +
>> +     return TRUE;
>> +
>> +done:
>> +     context_quit(context);
>> +     context->watch_id = 0;
>> +
>> +     return FALSE;
>> +}
>> +
>>  static void cmd_handler(const char *command, void *user_data)
>>  {
>>       struct context *context = user_data;
>> @@ -203,6 +268,9 @@ static void execute_context(struct context *context)
>>       if (context->hfp)
>>               hfp_gw_unref(context->hfp);
>>
>> +     if (context->hfp_hf)
>> +             hfp_hf_unref(context->hfp_hf);
>> +
>>       g_free(context);
>>  }
>>
>> @@ -275,24 +343,6 @@ static void test_register(gconstpointer data)
>>       execute_context(context);
>>  }
>>
>> -static gboolean send_pdu(gpointer user_data)
>> -{
>> -     struct context *context = user_data;
>> -     const struct test_pdu *pdu;
>> -     ssize_t len;
>> -
>> -     pdu = &context->data->pdu_list[context->pdu_offset++];
>> -
>> -     len = write(context->fd_server, pdu->data, pdu->size);
>> -     g_assert_cmpint(len, ==, pdu->size);
>> -
>> -     pdu = &context->data->pdu_list[context->pdu_offset];
>> -     if (pdu->fragmented)
>> -             g_idle_add(send_pdu, context);
>> -
>> -     return FALSE;
>> -}
>> -
>>  static void test_fragmented(gconstpointer data)
>>  {
>>       struct context *context = create_context(data);
>> @@ -404,6 +454,20 @@ static void check_string_2(struct hfp_gw_result *result,
>>       hfp_gw_send_result(context->hfp, HFP_RESULT_ERROR);
>>  }
>>
>> +static void test_hf_init(gconstpointer data)
>> +{
>> +     struct context *context = create_context(data);
>> +
>> +     context->hfp_hf = hfp_hf_new(context->fd_client);
>> +     g_assert(context->hfp_hf);
>> +     g_assert(hfp_hf_set_close_on_unref(context->hfp_hf, true));
>> +
>> +     hfp_hf_unref(context->hfp_hf);
>> +     context->hfp_hf = NULL;
>> +
>> +     execute_context(context);
>> +}
>> +
>>  int main(int argc, char *argv[])
>>  {
>>       g_test_init(&argc, &argv, NULL);
>> @@ -473,5 +537,7 @@ int main(int argc, char *argv[])
>>                       raw_pdu('\r'),
>>                       data_end());
>>
>> +     define_hf_test("/hfp/test_init", test_hf_init, NULL, NULL, data_end());
>
> I'd prefer if all hfp_hf tests were prefixed like this:
> "/hfp_hf/test_foo"
>
> This will allow to avoid doubling tests name like this one (there is already
> /hfp/test_init test).
>
OK

>> +
>>       return g_test_run();
>>  }
>>
>
> --
> Best regards,
> Szymon Janc

\Łukasz