[PATCH 03/18] gathdlc: add mechansim to detect '+++' escape sequence

[PATCH 03/18] gathdlc: add mechansim to detect '+++' escape sequence

[Guillaume Zajac]

[-- Attachment #1: Type: text/plain, Size: 4302 bytes --]

---
 gatchat/gathdlc.c |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 114 insertions(+), 0 deletions(-)

diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
index 7c45454..21e4533 100644
--- a/gatchat/gathdlc.c
+++ b/gatchat/gathdlc.c
@@ -50,6 +50,13 @@
 
 #define HDLC_FCS(fcs, c) crc_ccitt_byte(fcs, c)
 
+/* Amount of fiftieths of second to detect '+++'.
+ * Range is 0 to 0xff.
+ */
+#define ESC_TIME 	0x32
+
+#define GUARD_TIMEOUTS	1000	/* Pause time before and after '+++' sequence */
+
 struct _GAtHDLC {
 	gint ref_count;
 	GAtIO *io;
@@ -68,6 +75,12 @@ struct _GAtHDLC {
 	gboolean in_read_handler;
 	gboolean destroyed;
 	gboolean no_carrier_detect;
+	GAtSuspendFunc suspend_func;
+	gpointer suspend_data;
+	guint cmpt;
+	guint suspend_timeout;
+	guint pause_timeout;
+	gboolean paused;
 };
 
 static void hdlc_record(int fd, gboolean in, guint8 *data, guint16 length)
@@ -130,6 +143,47 @@ guint32 g_at_hdlc_get_recv_accm(GAtHDLC *hdlc)
 	return hdlc->recv_accm;
 }
 
+void g_at_hdlc_set_suspend_function(GAtHDLC *hdlc, GAtSuspendFunc func,
+							gpointer user_data)
+{
+	if (hdlc == NULL)
+		return;
+
+	hdlc->suspend_func = func;
+	hdlc->suspend_data = user_data;
+}
+
+static gboolean paused_timeout_cb(gpointer user_data)
+{
+	GAtHDLC *hdlc = user_data;
+
+	hdlc->paused = TRUE;
+
+	return FALSE;
+}
+
+static gboolean susp_timeout_cb(gpointer user_data)
+{
+	GAtHDLC *hdlc = user_data;
+
+	hdlc->cmpt = 0;
+
+	return FALSE;
+}
+
+static gboolean hdlc_suspend(gpointer user_data)
+{
+	GAtHDLC *hdlc = user_data;
+
+	g_at_io_set_write_handler(hdlc->io, NULL, NULL);
+	g_at_io_set_read_handler(hdlc->io, NULL, NULL);
+
+	if (hdlc->suspend_func)
+		hdlc->suspend_func(hdlc->suspend_data);
+
+	return FALSE;
+}
+
 static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
 {
 	GAtHDLC *hdlc = user_data;
@@ -142,6 +196,13 @@ static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
 
 	hdlc->in_read_handler = TRUE;
 
+	/*
+	 * We delete the the paused_timeout_cb or hdlc_suspend as soons as
+	 * we read a data.
+	 */
+	if (hdlc->pause_timeout > 0)
+		g_source_remove(hdlc->pause_timeout);
+
 	while (pos < len) {
 		/*
 		 * We try to detect NO CARRIER conditions here.  We
@@ -153,6 +214,21 @@ static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
 				hdlc->decode_offset == 0 && *buf == '\r')
 			break;
 
+		/*
+		 * If there was no character for 1 second we try to detect
+		 * the '+' character to suspend data call if 3 '+' are
+		 * detected in less than 20 * ESC_TIME milliseconds.
+		 */
+		if (*buf == '+' && hdlc->paused) {
+			if (hdlc->cmpt == 0)
+				hdlc->suspend_timeout = g_timeout_add (20 * ESC_TIME,
+							susp_timeout_cb, hdlc);
+			hdlc->cmpt++;
+		} else {
+			hdlc->cmpt = 0;
+			hdlc->paused = FALSE;
+		}
+
 		if (hdlc->decode_escape == TRUE) {
 			unsigned char val = *buf ^ HDLC_TRANS;
 
@@ -190,6 +266,9 @@ static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
 		}
 	}
 
+	if (hdlc->cmpt == 3)
+		goto suspend;
+
 out:
 	ring_buffer_drain(rbuf, pos);
 
@@ -197,6 +276,37 @@ out:
 
 	if (hdlc->destroyed)
 		g_free(hdlc);
+
+	/*
+	 * If there were no data pause for GUARD_TIMEOUTS ms,
+	 * we try again to check it.
+	 */
+	if (!hdlc->paused)
+		hdlc->pause_timeout = g_timeout_add (GUARD_TIMEOUTS,
+							paused_timeout_cb,
+							hdlc);
+
+	return;
+
+suspend:
+	/*
+	 * If the suspend timeout still exists,
+	 * delete it.
+	 */
+	if (hdlc->suspend_timeout > 0)
+		g_source_remove(hdlc->suspend_timeout);
+
+	/*
+	 * Restart the counter and reset the ring buffer.
+	 */
+	hdlc->cmpt = 0;
+	ring_buffer_reset(rbuf);
+
+	/*
+	 * Wait for another pause of GUARD_TIMEOUTS ms before returning to command mode.
+	 */
+	hdlc->paused = FALSE;
+	hdlc->pause_timeout = g_timeout_add (GUARD_TIMEOUTS, hdlc_suspend, hdlc);
 }
 
 GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
@@ -245,6 +355,10 @@ GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
 	hdlc->io = g_at_io_ref(io);
 	g_at_io_set_read_handler(hdlc->io, new_bytes, hdlc);
 
+	hdlc->cmpt = 0;
+
+	hdlc->paused = FALSE;
+
 	return hdlc;
 
 error:
-- 
1.7.1

Re: [PATCH 03/18] gathdlc: add mechansim to detect '+++' escape sequence

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 5383 bytes --]

Hi Guillaume,

On 03/25/2011 10:25 AM, Guillaume Zajac wrote:
> ---
>  gatchat/gathdlc.c |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 114 insertions(+), 0 deletions(-)
> 
> diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
> index 7c45454..21e4533 100644
> --- a/gatchat/gathdlc.c
> +++ b/gatchat/gathdlc.c
> @@ -50,6 +50,13 @@
>  
>  #define HDLC_FCS(fcs, c) crc_ccitt_byte(fcs, c)
>  
> +/* Amount of fiftieths of second to detect '+++'.
> + * Range is 0 to 0xff.
> + */
> +#define ESC_TIME 	0x32

Where is this coming from?  Is this supposed to be controlled by an
S-register?  If so, you might want to use a proper variable for this.

> +
> +#define GUARD_TIMEOUTS	1000	/* Pause time before and after '+++' sequence */
> +

This seems redundant to the ESC_TIME timeout...

>  struct _GAtHDLC {
>  	gint ref_count;
>  	GAtIO *io;
> @@ -68,6 +75,12 @@ struct _GAtHDLC {
>  	gboolean in_read_handler;
>  	gboolean destroyed;
>  	gboolean no_carrier_detect;
> +	GAtSuspendFunc suspend_func;
> +	gpointer suspend_data;
> +	guint cmpt;
> +	guint suspend_timeout;
> +	guint pause_timeout;

You never clean up these g_sources (e.g. in the case where GAtHDLC is
forcefully removed).  This is a big no no in a library, you must clean
up after yourself in all cases.

I would also replace the suspend_timeout with a GTimer directly.

> +	gboolean paused;
>  };
>  
>  static void hdlc_record(int fd, gboolean in, guint8 *data, guint16 length)
> @@ -130,6 +143,47 @@ guint32 g_at_hdlc_get_recv_accm(GAtHDLC *hdlc)
>  	return hdlc->recv_accm;
>  }
>  
> +void g_at_hdlc_set_suspend_function(GAtHDLC *hdlc, GAtSuspendFunc func,
> +							gpointer user_data)
> +{
> +	if (hdlc == NULL)
> +		return;
> +
> +	hdlc->suspend_func = func;
> +	hdlc->suspend_data = user_data;
> +}
> +
> +static gboolean paused_timeout_cb(gpointer user_data)
> +{
> +	GAtHDLC *hdlc = user_data;
> +
> +	hdlc->paused = TRUE;
> +
> +	return FALSE;
> +}
> +
> +static gboolean susp_timeout_cb(gpointer user_data)
> +{
> +	GAtHDLC *hdlc = user_data;
> +
> +	hdlc->cmpt = 0;
> +
> +	return FALSE;
> +}
> +
> +static gboolean hdlc_suspend(gpointer user_data)
> +{
> +	GAtHDLC *hdlc = user_data;
> +
> +	g_at_io_set_write_handler(hdlc->io, NULL, NULL);
> +	g_at_io_set_read_handler(hdlc->io, NULL, NULL);
> +
> +	if (hdlc->suspend_func)
> +		hdlc->suspend_func(hdlc->suspend_data);
> +
> +	return FALSE;
> +}
> +
>  static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
>  {
>  	GAtHDLC *hdlc = user_data;
> @@ -142,6 +196,13 @@ static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
>  
>  	hdlc->in_read_handler = TRUE;
>  
> +	/*
> +	 * We delete the the paused_timeout_cb or hdlc_suspend as soons as
> +	 * we read a data.
> +	 */
> +	if (hdlc->pause_timeout > 0)
> +		g_source_remove(hdlc->pause_timeout);
> +
>  	while (pos < len) {
>  		/*
>  		 * We try to detect NO CARRIER conditions here.  We
> @@ -153,6 +214,21 @@ static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
>  				hdlc->decode_offset == 0 && *buf == '\r')
>  			break;
>  
> +		/*
> +		 * If there was no character for 1 second we try to detect
> +		 * the '+' character to suspend data call if 3 '+' are
> +		 * detected in less than 20 * ESC_TIME milliseconds.
> +		 */
> +		if (*buf == '+' && hdlc->paused) {
> +			if (hdlc->cmpt == 0)
> +				hdlc->suspend_timeout = g_timeout_add (20 * ESC_TIME,
> +							susp_timeout_cb, hdlc);
> +			hdlc->cmpt++;
> +		} else {
> +			hdlc->cmpt = 0;
> +			hdlc->paused = FALSE;
> +		}
> +
>  		if (hdlc->decode_escape == TRUE) {
>  			unsigned char val = *buf ^ HDLC_TRANS;
>  
> @@ -190,6 +266,9 @@ static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
>  		}
>  	}
>  
> +	if (hdlc->cmpt == 3)
> +		goto suspend;
> +
>  out:
>  	ring_buffer_drain(rbuf, pos);
>  
> @@ -197,6 +276,37 @@ out:
>  
>  	if (hdlc->destroyed)
>  		g_free(hdlc);
> +
> +	/*
> +	 * If there were no data pause for GUARD_TIMEOUTS ms,
> +	 * we try again to check it.
> +	 */
> +	if (!hdlc->paused)
> +		hdlc->pause_timeout = g_timeout_add (GUARD_TIMEOUTS,
> +							paused_timeout_cb,
> +							hdlc);
> +
> +	return;
> +
> +suspend:
> +	/*
> +	 * If the suspend timeout still exists,
> +	 * delete it.
> +	 */
> +	if (hdlc->suspend_timeout > 0)
> +		g_source_remove(hdlc->suspend_timeout);
> +
> +	/*
> +	 * Restart the counter and reset the ring buffer.
> +	 */
> +	hdlc->cmpt = 0;
> +	ring_buffer_reset(rbuf);

This sounds like a really bad idea.  What are you trying to accomplish here?

> +
> +	/*
> +	 * Wait for another pause of GUARD_TIMEOUTS ms before returning to command mode.
> +	 */
> +	hdlc->paused = FALSE;
> +	hdlc->pause_timeout = g_timeout_add (GUARD_TIMEOUTS, hdlc_suspend, hdlc);
>  }
>  
>  GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
> @@ -245,6 +355,10 @@ GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
>  	hdlc->io = g_at_io_ref(io);
>  	g_at_io_set_read_handler(hdlc->io, new_bytes, hdlc);
>  
> +	hdlc->cmpt = 0;
> +
> +	hdlc->paused = FALSE;
> +

In general it isn't necessary to initialize variables to zero since we
use g_try_new0 which zeros the structure already.

>  	return hdlc;
>  
>  error:

Regards,
-Denis

Re: [PATCH 03/18] gathdlc: add mechansim to detect '+++' escape sequence

[Guillaume Zajac]

[-- Attachment #1: Type: text/plain, Size: 6371 bytes --]

Hi Denis,

On 30/03/2011 22:52, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 03/25/2011 10:25 AM, Guillaume Zajac wrote:
>> ---
>>   gatchat/gathdlc.c |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 114 insertions(+), 0 deletions(-)
>>
>> diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
>> index 7c45454..21e4533 100644
>> --- a/gatchat/gathdlc.c
>> +++ b/gatchat/gathdlc.c
>> @@ -50,6 +50,13 @@
>>
>>   #define HDLC_FCS(fcs, c) crc_ccitt_byte(fcs, c)
>>
>> +/* Amount of fiftieths of second to detect '+++'.
>> + * Range is 0 to 0xff.
>> + */
>> +#define ESC_TIME 	0x32
> Where is this coming from?  Is this supposed to be controlled by an
> S-register?  If so, you might want to use a proper variable for this.
>> +
>> +#define GUARD_TIMEOUTS	1000	/* Pause time before and after '+++' sequence */
>> +
> This seems redundant to the ESC_TIME timeout...

GUARD_TIMEOUTS comes from S12 register (same as escape character comes 
from S2 register), however it is not implemented in GAtServer.
I don't think I can access to v250 registers from HDLC layer.

Then, I thought I needed to have minimum time e.g. ESC_TIME to detect 3 
'+' characters and a GUARD_TIMEOUTS, that's why I used 2 variables. 
However I don't find any document speaking about ESC_TIME. So I will 
keep only GUARD_TIMEOUT.

>>   struct _GAtHDLC {
>>   	gint ref_count;
>>   	GAtIO *io;
>> @@ -68,6 +75,12 @@ struct _GAtHDLC {
>>   	gboolean in_read_handler;
>>   	gboolean destroyed;
>>   	gboolean no_carrier_detect;
>> +	GAtSuspendFunc suspend_func;
>> +	gpointer suspend_data;
>> +	guint cmpt;
>> +	guint suspend_timeout;
>> +	guint pause_timeout;
> You never clean up these g_sources (e.g. in the case where GAtHDLC is
> forcefully removed).  This is a big no no in a library, you must clean
> up after yourself in all cases.
>
> I would also replace the suspend_timeout with a GTimer directly.
>

So I will use GTimer and make sure I correctly clean it up.

>> +	gboolean paused;
>>   };
>>
>>   static void hdlc_record(int fd, gboolean in, guint8 *data, guint16 length)
>> @@ -130,6 +143,47 @@ guint32 g_at_hdlc_get_recv_accm(GAtHDLC *hdlc)
>>   	return hdlc->recv_accm;
>>   }
>>
>> +void g_at_hdlc_set_suspend_function(GAtHDLC *hdlc, GAtSuspendFunc func,
>> +							gpointer user_data)
>> +{
>> +	if (hdlc == NULL)
>> +		return;
>> +
>> +	hdlc->suspend_func = func;
>> +	hdlc->suspend_data = user_data;
>> +}
>> +
>> +static gboolean paused_timeout_cb(gpointer user_data)
>> +{
>> +	GAtHDLC *hdlc = user_data;
>> +
>> +	hdlc->paused = TRUE;
>> +
>> +	return FALSE;
>> +}
>> +
>> +static gboolean susp_timeout_cb(gpointer user_data)
>> +{
>> +	GAtHDLC *hdlc = user_data;
>> +
>> +	hdlc->cmpt = 0;
>> +
>> +	return FALSE;
>> +}
>> +
>> +static gboolean hdlc_suspend(gpointer user_data)
>> +{
>> +	GAtHDLC *hdlc = user_data;
>> +
>> +	g_at_io_set_write_handler(hdlc->io, NULL, NULL);
>> +	g_at_io_set_read_handler(hdlc->io, NULL, NULL);
>> +
>> +	if (hdlc->suspend_func)
>> +		hdlc->suspend_func(hdlc->suspend_data);
>> +
>> +	return FALSE;
>> +}
>> +
>>   static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
>>   {
>>   	GAtHDLC *hdlc = user_data;
>> @@ -142,6 +196,13 @@ static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
>>
>>   	hdlc->in_read_handler = TRUE;
>>
>> +	/*
>> +	 * We delete the the paused_timeout_cb or hdlc_suspend as soons as
>> +	 * we read a data.
>> +	 */
>> +	if (hdlc->pause_timeout>  0)
>> +		g_source_remove(hdlc->pause_timeout);
>> +
>>   	while (pos<  len) {
>>   		/*
>>   		 * We try to detect NO CARRIER conditions here.  We
>> @@ -153,6 +214,21 @@ static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
>>   				hdlc->decode_offset == 0&&  *buf == '\r')
>>   			break;
>>
>> +		/*
>> +		 * If there was no character for 1 second we try to detect
>> +		 * the '+' character to suspend data call if 3 '+' are
>> +		 * detected in less than 20 * ESC_TIME milliseconds.
>> +		 */
>> +		if (*buf == '+'&&  hdlc->paused) {
>> +			if (hdlc->cmpt == 0)
>> +				hdlc->suspend_timeout = g_timeout_add (20 * ESC_TIME,
>> +							susp_timeout_cb, hdlc);
>> +			hdlc->cmpt++;
>> +		} else {
>> +			hdlc->cmpt = 0;
>> +			hdlc->paused = FALSE;
>> +		}
>> +
>>   		if (hdlc->decode_escape == TRUE) {
>>   			unsigned char val = *buf ^ HDLC_TRANS;
>>
>> @@ -190,6 +266,9 @@ static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
>>   		}
>>   	}
>>
>> +	if (hdlc->cmpt == 3)
>> +		goto suspend;
>> +
>>   out:
>>   	ring_buffer_drain(rbuf, pos);
>>
>> @@ -197,6 +276,37 @@ out:
>>
>>   	if (hdlc->destroyed)
>>   		g_free(hdlc);
>> +
>> +	/*
>> +	 * If there were no data pause for GUARD_TIMEOUTS ms,
>> +	 * we try again to check it.
>> +	 */
>> +	if (!hdlc->paused)
>> +		hdlc->pause_timeout = g_timeout_add (GUARD_TIMEOUTS,
>> +							paused_timeout_cb,
>> +							hdlc);
>> +
>> +	return;
>> +
>> +suspend:
>> +	/*
>> +	 * If the suspend timeout still exists,
>> +	 * delete it.
>> +	 */
>> +	if (hdlc->suspend_timeout>  0)
>> +		g_source_remove(hdlc->suspend_timeout);
>> +
>> +	/*
>> +	 * Restart the counter and reset the ring buffer.
>> +	 */
>> +	hdlc->cmpt = 0;
>> +	ring_buffer_reset(rbuf);
> This sounds like a really bad idea.  What are you trying to accomplish here?

If I don't reset ring_buffer, I am receiving "+++" on GAtServer when I 
return to command mode and it is blocking following AT commands.
I can't send next ATH0 or ATO0. Do you have any idea how to prevent 
receiving GAtServer to receive it?

>> +
>> +	/*
>> +	 * Wait for another pause of GUARD_TIMEOUTS ms before returning to command mode.
>> +	 */
>> +	hdlc->paused = FALSE;
>> +	hdlc->pause_timeout = g_timeout_add (GUARD_TIMEOUTS, hdlc_suspend, hdlc);
>>   }
>>
>>   GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
>> @@ -245,6 +355,10 @@ GAtHDLC *g_at_hdlc_new_from_io(GAtIO *io)
>>   	hdlc->io = g_at_io_ref(io);
>>   	g_at_io_set_read_handler(hdlc->io, new_bytes, hdlc);
>>
>> +	hdlc->cmpt = 0;
>> +
>> +	hdlc->paused = FALSE;
>> +
> In general it isn't necessary to initialize variables to zero since we
> use g_try_new0 which zeros the structure already.

Ok

Kind regards,
Guillaume

Re: [PATCH 03/18] gathdlc: add mechansim to detect '+++' escape sequence

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

Hi Guillaume,

>>> +suspend:
>>> +    /*
>>> +     * If the suspend timeout still exists,
>>> +     * delete it.
>>> +     */
>>> +    if (hdlc->suspend_timeout>  0)
>>> +        g_source_remove(hdlc->suspend_timeout);
>>> +
>>> +    /*
>>> +     * Restart the counter and reset the ring buffer.
>>> +     */
>>> +    hdlc->cmpt = 0;
>>> +    ring_buffer_reset(rbuf);
>> This sounds like a really bad idea.  What are you trying to accomplish
>> here?
> 
> If I don't reset ring_buffer, I am receiving "+++" on GAtServer when I
> return to command mode and it is blocking following AT commands.
> I can't send next ATH0 or ATO0. Do you have any idea how to prevent
> receiving GAtServer to receive it?

So two issues:

- why are you using ring_buffer_reset instead of ring_buffer_drain(buf, 3)?

- Why are you doing this here instead of waiting for the guard timeout
to expire after +++ has been detected?

> 
>>> +
>>> +    /*
>>> +     * Wait for another pause of GUARD_TIMEOUTS ms before returning
>>> to command mode.
>>> +     */
>>> +    hdlc->paused = FALSE;
>>> +    hdlc->pause_timeout = g_timeout_add (GUARD_TIMEOUTS,
>>> hdlc_suspend, hdlc);
>>>   }
>>>

Regards,
-Denis

Re: [PATCH 03/18] gathdlc: add mechansim to detect '+++' escape sequence

[Guillaume Zajac]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

Hi Denis,

On 31/03/2011 17:25, Denis Kenzior wrote:
> Hi Guillaume,
>
>>>> +suspend:
>>>> +    /*
>>>> +     * If the suspend timeout still exists,
>>>> +     * delete it.
>>>> +     */
>>>> +    if (hdlc->suspend_timeout>   0)
>>>> +        g_source_remove(hdlc->suspend_timeout);
>>>> +
>>>> +    /*
>>>> +     * Restart the counter and reset the ring buffer.
>>>> +     */
>>>> +    hdlc->cmpt = 0;
>>>> +    ring_buffer_reset(rbuf);
>>> This sounds like a really bad idea.  What are you trying to accomplish
>>> here?
>> If I don't reset ring_buffer, I am receiving "+++" on GAtServer when I
>> return to command mode and it is blocking following AT commands.
>> I can't send next ATH0 or ATO0. Do you have any idea how to prevent
>> receiving GAtServer to receive it?
> So two issues:
>
> - why are you using ring_buffer_reset instead of ring_buffer_drain(buf, 3)?

I didn't think about it :)

> - Why are you doing this here instead of waiting for the guard timeout
> to expire after +++ has been detected?
>

I didn't think that an HDLC frame can contain +++++...
I will change this.

Thanks.

Kind regards,
Guillaume