Hi Kristen,
> gatchat/gathdlc.c | 68 +++++++++++++++++++++++++++-------------------------
> 1 files changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
> index 19df9c6..7832350 100644
> --- a/gatchat/gathdlc.c
> +++ b/gatchat/gathdlc.c
> @@ -53,11 +53,12 @@ static void new_bytes(GAtHDLC *hdlc)
> {
> unsigned int len = ring_buffer_len(hdlc->read_buffer);
> unsigned char *buf = ring_buffer_read_ptr(hdlc->read_buffer, 0);
> + unsigned int wrap = ring_buffer_len_no_wrap(hdlc->read_buffer);
to keep the design in sync with how gatchat.c does the variable order,
please put wrap before *buf.
> unsigned char val;
> unsigned int pos = 0;
>
> while (pos < len) {
> - if (buf[pos] == 0x7e) {
> + if (*buf == 0x7e) {
> if (hdlc->receive_func && hdlc->decode_offset > 2 &&
> hdlc->decode_fcs == 0xf0b8) {
> hdlc->receive_func(hdlc->decode_buffer,
> @@ -68,22 +69,33 @@ static void new_bytes(GAtHDLC *hdlc)
> hdlc->decode_fcs = 0xffff;
> hdlc->decode_offset = 0;
> pos++;
> + buf++;
> + if (pos == wrap)
> + buf = ring_buffer_read_ptr(hdlc->read_buffer,
> + pos);
> continue;
> }
>
> - if (buf[pos] == 0x7d) {
> + if (*buf == 0x7d) {
> if (pos + 2 > len)
> break;
> pos++;
> - val = buf[pos] ^ 0x20;
> + buf++;
> + if (pos == wrap)
> + buf = ring_buffer_read_ptr(hdlc->read_buffer,
> + pos);
> + val = *buf ^ 0x20;
> } else
> - val = buf[pos];
> + val = *buf;
>
> hdlc->decode_buffer[hdlc->decode_offset] = val;
> hdlc->decode_fcs = crc_ccitt_byte(hdlc->decode_fcs, val);
>
> hdlc->decode_offset++;
> pos++;
> + buf++;
> + if (pos == wrap)
> + buf = ring_buffer_read_ptr(hdlc->read_buffer, pos);
> }
>
> ring_buffer_drain(hdlc->read_buffer, pos);
This part looks fine. However we repeat the following code here three
times:
pos++;
buf++;
if (pos == wrap)
buf = ring_buffer_read_ptr(hdlc->read_buffer, pos);
We might need to think about a bit of restructure or using a define.
> @@ -302,47 +314,37 @@ static void wakeup_write(GAtHDLC *hdlc)
> can_write_data, hdlc, write_watch_destroy);
> }
>
> -static inline void hdlc_put(GAtHDLC *hdlc, guint8 *buf, gsize *pos, guint8 c)
> +static inline void hdlc_buffer_put_char(struct ring_buffer *buffer,
> + const char val)
> {
> - gsize i = *pos;
> + ring_buffer_write(buffer, &val, 1);
> +}
Please don't do this. It has a massive performance impact. Please check
the ring_buffer_write() function and see what it is doing. Just writing
a char is bad. Really bad.
If you want to write a char then adding ring_buffer_write_char()
function is the way to go here.
> +static inline void hdlc_put(GAtHDLC *hdlc, guint8 c)
> +{
> if (c == 0x7e || c == 0x7d) {
> - buf[i++] = 0x7d;
> - buf[i++] = c ^ 0x20;
> + hdlc_buffer_put_char(hdlc->write_buffer, 0x7d);
> + hdlc_buffer_put_char(hdlc->write_buffer, c ^ 0x20);
> } else
> - buf[i++] = c;
> -
> - *pos = i;
> + hdlc_buffer_put_char(hdlc->write_buffer, c);
> }
>
> gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
> {
> - unsigned char *buf;
> - unsigned int space, i = 0;
> + unsigned int i = 0;
> guint16 fcs = 0xffff;
> - gsize pos;
>
> - do {
> - space = ring_buffer_avail_no_wrap(hdlc->write_buffer);
> - if (space == 0)
> - break;
> + hdlc_buffer_put_char(hdlc->write_buffer, 0x7e);
>
> - buf = ring_buffer_write_ptr(hdlc->write_buffer);
> - pos = 0;
> -
> - while (size--) {
> - fcs = crc_ccitt_byte(fcs, data[i]);
> - hdlc_put(hdlc, buf, &pos, data[i++]);
> - }
> -
> - fcs ^= 0xffff;
> - hdlc_put(hdlc, buf, &pos, fcs & 0xff);
> - hdlc_put(hdlc, buf, &pos, fcs >> 8);
> -
> - buf[pos++] = 0x7e;
> + while (size--) {
> + fcs = crc_ccitt_byte(fcs, data[i]);
> + hdlc_put(hdlc, data[i++]);
> + }
>
> - ring_buffer_write_advance(hdlc->write_buffer, pos);
> - } while (0);
> + fcs ^= 0xffff;
> + hdlc_put(hdlc, fcs & 0xff);
> + hdlc_put(hdlc, fcs >> 8);
> + hdlc_buffer_put_char(hdlc->write_buffer, 0x7e);
>
> wakeup_write(hdlc);
The only problem area with my original code is that when we are close
before the wrapping and have to write an encoded character that has one
byte before and one byte after the wrapping. Every other case should
have just worked fine.
Regards
Marcel
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
