[patch 1/6] Add PPP protocol support with HDLC framing
This patch implements the basic PPP protocol. LCP, NCP etc. are handled in a
different patch.
Index: ofono/Makefile.am
===
--- ofono.orig/Makefile.am 2010-03-10 16:58:09.915955860 -0800
+++ ofono/Makefile.am 2010-03-10 16:58:12.039961039 -0800
@@ -55,7 +55,9 @@
gatchat/gattty.h gatchat/gattty.c \
gatchat/gatutil.h gatchat/gatutil.c \
gatchat/gat.h \
- gatchat/gatserver.h gatchat/gatserver.c
+ gatchat/gatserver.h gatchat/gatserver.c \
+ gatchat/gatppp.c gatchat/gatppp.h \
+ gatchat/gatppp_internal.h
udev_files = plugins/ofono.rules
Index: ofono/gatchat/gatppp_internal.h
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ ofono/gatchat/gatppp_internal.h 2010-03-10 16:58:12.039961039 -0800
@@ -0,0 +1,97 @@
+/*
+ *
+ * PPP library with GLib integration
+ *
+ * Copyright (C) 2010 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifndef __G_AT_PPP_INTERNAL_H
+#define __G_AT_PPP_INTERNAL_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define DEFAULT_MRU1500
+#define BUFFERSZ DEFAULT_MRU*2
+#define DEFAULT_ACCM 0x
+#define PPP_ESC0x7d
+#define PPP_FLAG_SEQ 0x7e
+#define PPP_ADDR_FIELD 0xff
+#define PPP_CTRL 0x03
+#define LCP_PROTOCOL 0xc021
+#define CHAP_PROTOCOL 0xc223
+#define PPP_HEADROOM 2
+#define HDLC_HEADROOM 3
+#define HDLC_TAIL 3
+#define MD55
+
+enum PPP_PHASE {
+ DEAD = 0,
+ ESTABLISHMENT,
+ AUTHENTICATION,
+ NETWORK,
+ TERMINATION,
+};
+
+enum PPP_EVENTS {
+ PPP_UP = 1,
+ PPP_OPENED,
+ PPP_SUCCESS,
+ PPP_NONE,
+ PPP_CLOSING,
+ PPP_FAIL,
+ PPP_DOWN
+};
+
+struct ppp_packet_handler {
+ guint16 proto;
+ void (*handler)(gpointer priv, guint8 *packet);
+ gpointer priv;
+};
+
+#define ppp_generate_event(l, e) \
+ __ppp_generate_event(l, e)
+
+#define ppp_transmit(l, p, i) \
+ __ppp_transmit(l, p, i)
+
+#define ppp_register_packet_handler(h) \
+ __ppp_register_packet_handler(h)
+
+#define ppp_info(packet) \
+ (packet+4)
+
+#define ppp_proto(packet) \
+ (ntohs(*((guint16*)(packet+2
+
+void __ppp_generate_event(struct ppp_link *link, guint event);
+void __ppp_register_packet_handler(struct ppp_packet_handler *handler);
+void __ppp_transmit(struct ppp_link *link, guint8 *packet, guint infolen);
+void __ppp_set_auth(struct ppp_link *link, guint8 *auth_data);
+void __ppp_set_recv_accm(struct ppp_link *link, guint32 accm);
+guint32 __ppp_get_xmit_accm(struct ppp_link *link);
+void __ppp_set_pfc(struct ppp_link *link, gboolean pfc);
+gboolean __ppp_get_pfc(struct ppp_link *link);
+void __ppp_set_acfc(struct ppp_link *link, gboolean acfc);
+gboolean __ppp_get_acfc(struct ppp_link *link);
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __G_AT_PPP_INTERNAL_H */
+
Index: ofono/gatchat/gatppp.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ ofono/gatchat/gatppp.c 2010-03-10 16:58:12.039961039 -0800
@@ -0,0 +1,557 @@
+/*
+ *
+ * PPP library with GLib integration
+ *
+ * Copyright (C) 2010 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include
+#endif
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
[patch 1/6] Add PPP protocol support with HDLC framing
This patch implements the basic PPP protocol. LCP, NCP etc. are handled in a
different patch.
---
Makefile.am |4
gatchat/gatppp.c | 552 ++
gatchat/gatppp.h | 59
gatchat/gatppp_internal.h | 97
4 files changed, 711 insertions(+), 1 deletion(-)
Index: ofono/Makefile.am
===
--- ofono.orig/Makefile.am 2010-03-16 15:16:53.536470034 -0700
+++ ofono/Makefile.am 2010-03-16 15:33:46.144468807 -0700
@@ -55,7 +55,9 @@
gatchat/gattty.h gatchat/gattty.c \
gatchat/gatutil.h gatchat/gatutil.c \
gatchat/gat.h \
- gatchat/gatserver.h gatchat/gatserver.c
+ gatchat/gatserver.h gatchat/gatserver.c \
+ gatchat/gatppp.c gatchat/gatppp.h \
+ gatchat/gatppp_internal.h
udev_files = plugins/ofono.rules
Index: ofono/gatchat/gatppp_internal.h
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ ofono/gatchat/gatppp_internal.h 2010-03-16 15:33:46.144468807 -0700
@@ -0,0 +1,97 @@
+/*
+ *
+ * PPP library with GLib integration
+ *
+ * Copyright (C) 2010 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+#define DEFAULT_MRU1500
+#define BUFFERSZ DEFAULT_MRU*2
+#define DEFAULT_ACCM 0x
+#define PPP_ESC0x7d
+#define PPP_FLAG_SEQ 0x7e
+#define PPP_ADDR_FIELD 0xff
+#define PPP_CTRL 0x03
+#define LCP_PROTOCOL 0xc021
+#define CHAP_PROTOCOL 0xc223
+#define PPP_HEADROOM 2
+#define HDLC_HEADROOM 3
+#define HDLC_TAIL 3
+#define MD55
+
+typedef enum _GAthase {
+ DEAD = 0,
+ ESTABLISHMENT,
+ AUTHENTICATION,
+ NETWORK,
+ TERMINATION,
+} GAthase;
+
+typedef enum _GAtPPPEvents {
+ PPP_UP = 1,
+ PPP_OPENED,
+ PPP_SUCCESS,
+ PPP_NONE,
+ PPP_CLOSING,
+ PPP_FAIL,
+ PPP_DOWN
+} GAtPPPEvents;
+
+struct ppp_packet_handler {
+ guint16 proto;
+ void (*handler)(gpointer priv, guint8 *packet);
+ gpointer priv;
+};
+
+#define ppp_info(packet) \
+ (packet+4)
+
+#define ppp_proto(packet) \
+ (ntohs(*((guint16*)(packet+2
+
+struct _GAtPPP {
+ gint ref_count;
+ GAthase phase;
+ guint8 buffer[BUFFERSZ];
+ int index;
+ gint mru;
+ guint16 auth_proto;
+ char user_name[256];
+ char passwd[256];
+ gboolean pfc;
+ gboolean acfc;
+ guint32 xmit_accm[8];
+ guint32 recv_accm;
+ GIOChannel *modem;
+ GQueue *event_queue;
+ GQueue *recv_queue;
+ GAtPPPConnectFunc connect_cb;
+ gpointer connect_data;
+ GAtPPPDisconnectFunc disconnect_cb;
+ gpointer disconnect_data;
+ gint modem_watch;
+};
+
+void __ppp_generate_event(GAtPPP *ppp, GAtPPPEvents event);
+void __ppp_register_packet_handler(struct ppp_packet_handler *handler);
+void __ppp_transmit(GAtPPP *ppp, guint8 *packet, guint infolen);
+void __ppp_set_auth(GAtPPP *ppp, guint8 *auth_data);
+void __ppp_set_recv_accm(GAtPPP *ppp, guint32 accm);
+guint32 __ppp_get_xmit_accm(GAtPPP *ppp);
+void __ppp_set_pfc(GAtPPP *ppp, gboolean pfc);
+gboolean __ppp_get_pfc(GAtPPP *ppp);
+void __ppp_set_acfc(GAtPPP *ppp, gboolean acfc);
+gboolean __ppp_get_acfc(GAtPPP *ppp);
Index: ofono/gatchat/gatppp.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ ofono/gatchat/gatppp.c 2010-03-16 15:34:30.492565338 -0700
@@ -0,0 +1,552 @@
+/*
+ *
+ * PPP library with GLib integration
+ *
+ * Copyright (C) 2010 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * G
[patch 1/6] Add PPP protocol support with HDLC framing
This patch implements the basic PPP protocol. LCP, NCP etc. are handled in a
different patch.
---
Makefile.am |4
gatchat/gatppp.c | 133
gatchat/gatppp.h | 59 +++
gatchat/ppp.c| 455 +++
gatchat/ppp.h| 130 +++
5 files changed, 780 insertions(+), 1 deletion(-)
Index: ofono/Makefile.am
===
--- ofono.orig/Makefile.am 2010-03-19 11:49:00.060469174 -0700
+++ ofono/Makefile.am 2010-03-19 11:49:02.652468489 -0700
@@ -55,7 +55,9 @@
gatchat/gattty.h gatchat/gattty.c \
gatchat/gatutil.h gatchat/gatutil.c \
gatchat/gat.h \
- gatchat/gatserver.h gatchat/gatserver.c
+ gatchat/gatserver.h gatchat/gatserver.c \
+ gatchat/gatppp.c gatchat/gatppp.h \
+ gatchat/ppp.c gatchat/ppp.h
udev_files = plugins/ofono.rules
Index: ofono/gatchat/gatppp.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ ofono/gatchat/gatppp.c 2010-03-19 11:49:02.652468489 -0700
@@ -0,0 +1,133 @@
+/*
+ *
+ * PPP library with GLib integration
+ *
+ * Copyright (C) 2009-2010 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include
+#endif
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#include "gatutil.h"
+#include "gatppp.h"
+#include "ppp.h"
+
+/* Administrative Open */
+void g_at_ppp_open(GAtPPP *ppp)
+{
+ /* send an OPEN event to the lcp layer */
+}
+
+void g_at_ppp_set_connect_function(GAtPPP *ppp,
+ GAtPPPConnectFunc callback, gpointer user_data)
+{
+ ppp->connect_cb = callback;
+ ppp->connect_data = user_data;
+}
+
+void g_at_ppp_set_disconnect_function(GAtPPP *ppp,
+ GAtPPPDisconnectFunc callback,
+ gpointer user_data)
+{
+ ppp->disconnect_cb = callback;
+ ppp->disconnect_data = user_data;
+}
+
+void g_at_ppp_shutdown(GAtPPP *ppp)
+{
+ /* close the ppp */
+ ppp_close(ppp);
+
+ /* clean up all the queues */
+ g_queue_free(ppp->event_queue);
+ g_queue_free(ppp->recv_queue);
+
+ /* cleanup modem channel */
+ g_source_remove(ppp->modem_watch);
+ g_io_channel_unref(ppp->modem);
+}
+
+void g_at_ppp_ref(GAtPPP *ppp)
+{
+ g_atomic_int_inc(&ppp->ref_count);
+}
+
+void g_at_ppp_unref(GAtPPP *ppp)
+{
+ if (g_atomic_int_dec_and_test(&ppp->ref_count)) {
+ g_at_ppp_shutdown(ppp);
+ g_free(ppp);
+ }
+}
+
+GAtPPP *g_at_ppp_new(GIOChannel *modem)
+{
+ GAtPPP *ppp;
+
+ ppp = g_try_malloc0(sizeof(GAtPPP));
+ if (!ppp)
+ return NULL;
+
+ ppp->modem = g_io_channel_ref(modem);
+ if (!g_at_util_setup_io(ppp->modem, G_IO_FLAG_NONBLOCK)) {
+ g_io_channel_unref(modem);
+ g_free(ppp);
+ return NULL;
+ }
+ g_io_channel_set_buffered(modem, FALSE);
+
+ ppp->ref_count = 1;
+
+ /* set options to defaults */
+ ppp->mru = DEFAULT_MRU;
+ ppp->recv_accm = DEFAULT_ACCM;
+ ppp->xmit_accm[0] = DEFAULT_ACCM;
+ ppp->xmit_accm[3] = 0x6000; /* 0x7d, 0x7e */
+ ppp->pfc = FALSE;
+ ppp->acfc = FALSE;
+
+ /* allocate the queues */
+ ppp->event_queue = g_queue_new();
+ ppp->recv_queue = g_queue_new();
+
+ ppp->index = 0;
+
+ /* initialize the lcp state */
+
+
+ /* initialize the autentication state */
+
+
+ /* intialize the network state */
+
+ /* start listening for packets from the modem */
+ ppp->modem_watch = g_io_add_watch(modem,
+ G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+ ppp_cb, ppp);
+
+ return ppp;
+}
Index: ofono/gatchat/gatppp.h
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ ofono/gatchat/gatppp.h 2010-03-19 11:49:02.652468489 -0700
@
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Kristen,
> This patch implements the basic PPP protocol. LCP, NCP etc. are handled in
> a different patch.
>
> Index: ofono/Makefile.am
>
===
> --- ofono.orig/Makefile.am2010-03-10 16:58:09.915955860 -0800
> +++ ofono/Makefile.am 2010-03-10 16:58:12.039961039 -0800
> @@ -55,7 +55,9 @@
> gatchat/gattty.h gatchat/gattty.c \
> gatchat/gatutil.h gatchat/gatutil.c \
> gatchat/gat.h \
> - gatchat/gatserver.h gatchat/gatserver.c
> + gatchat/gatserver.h gatchat/gatserver.c \
> + gatchat/gatppp.c gatchat/gatppp.h \
> + gatchat/gatppp_internal.h
>
> udev_files = plugins/ofono.rules
>
> Index: ofono/gatchat/gatppp_internal.h
>
===
> --- /dev/null 1970-01-01 00:00:00.0 +
> +++ ofono/gatchat/gatppp_internal.h 2010-03-10 16:58:12.039961039 -0800
> @@ -0,0 +1,97 @@
> +/*
> + *
> + * PPP library with GLib integration
> + *
> + * Copyright (C) 2010 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
> USA + *
> + */
> +
> +#ifndef __G_AT_PPP_INTERNAL_H
> +#define __G_AT_PPP_INTERNAL_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#define DEFAULT_MRU 1500
> +#define BUFFERSZ DEFAULT_MRU*2
> +#define DEFAULT_ACCM 0x
> +#define PPP_ESC 0x7d
> +#define PPP_FLAG_SEQ 0x7e
> +#define PPP_ADDR_FIELD 0xff
> +#define PPP_CTRL 0x03
> +#define LCP_PROTOCOL 0xc021
> +#define CHAP_PROTOCOL0xc223
> +#define PPP_HEADROOM 2
> +#define HDLC_HEADROOM3
> +#define HDLC_TAIL3
> +#define MD5 5
> +
> +enum PPP_PHASE {
> + DEAD = 0,
> + ESTABLISHMENT,
> + AUTHENTICATION,
> + NETWORK,
> + TERMINATION,
> +};
> +
> +enum PPP_EVENTS {
> + PPP_UP = 1,
> + PPP_OPENED,
> + PPP_SUCCESS,
> + PPP_NONE,
> + PPP_CLOSING,
> + PPP_FAIL,
> + PPP_DOWN
> +};
> +
> +struct ppp_packet_handler {
> + guint16 proto;
> + void (*handler)(gpointer priv, guint8 *packet);
> + gpointer priv;
> +};
> +
> +#define ppp_generate_event(l, e) \
> + __ppp_generate_event(l, e)
Why do we need this define?
> +
> +#define ppp_transmit(l, p, i) \
> + __ppp_transmit(l, p, i)
> +
and this?
> +#define ppp_register_packet_handler(h) \
> + __ppp_register_packet_handler(h)
or this?
> +
> +#define ppp_info(packet) \
> + (packet+4)
> +
> +#define ppp_proto(packet) \
> + (ntohs(*((guint16*)(packet+2
> +
> +void __ppp_generate_event(struct ppp_link *link, guint event);
> +void __ppp_register_packet_handler(struct ppp_packet_handler *handler);
> +void __ppp_transmit(struct ppp_link *link, guint8 *packet, guint infolen);
> +void __ppp_set_auth(struct ppp_link *link, guint8 *auth_data);
> +void __ppp_set_recv_accm(struct ppp_link *link, guint32 accm);
> +guint32 __ppp_get_xmit_accm(struct ppp_link *link);
> +void __ppp_set_pfc(struct ppp_link *link, gboolean pfc);
> +gboolean __ppp_get_pfc(struct ppp_link *link);
> +void __ppp_set_acfc(struct ppp_link *link, gboolean acfc);
> +gboolean __ppp_get_acfc(struct ppp_link *link);
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* __G_AT_PPP_INTERNAL_H */
> +
> Index: ofono/gatchat/gatppp.c
>
===
> --- /dev/null 1970-01-01 00:00:00.0 +
> +++ ofono/gatchat/gatppp.c2010-03-10 16:58:12.039961039 -0800
> @@ -0,0 +1,557 @@
> +/*
> + *
> + * PPP library with GLib integration
> + *
> + * Copyright (C) 2010 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Publ
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Kristen, > +struct ppp_link * g_at_ppp_new(GIOChannel *modem); > +void g_at_ppp_open(struct ppp_link *link); > +void g_at_ppp_set_connect_function(struct ppp_link *link, > +GAtPPPConnectFunc callback, gpointer user_data); > +void g_at_ppp_set_disconnect_function(struct ppp_link *link, > + GAtPPPDisconnectFunc callback, > + gpointer user_data); > +void g_at_ppp_shutdown(struct ppp_link *link); > +void g_at_ppp_ref(struct ppp_link *link); > +void g_at_ppp_unref(struct ppp_link *link); Almost forgot, let us not use struct ppp_link here, but instead use GAtPpp *ppp as the parameter. Again, mostly for consistency with how glib / gatchat does this. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Denis, > > +struct ppp_link * g_at_ppp_new(GIOChannel *modem); > > +void g_at_ppp_open(struct ppp_link *link); > > +void g_at_ppp_set_connect_function(struct ppp_link *link, > > + GAtPPPConnectFunc callback, gpointer user_data); > > +void g_at_ppp_set_disconnect_function(struct ppp_link *link, > > + GAtPPPDisconnectFunc callback, > > + gpointer user_data); > > +void g_at_ppp_shutdown(struct ppp_link *link); > > +void g_at_ppp_ref(struct ppp_link *link); > > +void g_at_ppp_unref(struct ppp_link *link); > > Almost forgot, let us not use struct ppp_link here, but instead use GAtPpp > *ppp as the parameter. Again, mostly for consistency with how glib / gatchat > does this. I am actually okay with GAtPPP since it looks much nicer. True camel case is making it harder to read. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [patch 1/6] Add PPP protocol support with HDLC framing
On Thu, 11 Mar 2010 20:17:49 -0600
Denis Kenzior wrote:
>
> > + } else {
> > + /* store last flag character */
> > + link->buffer[link->index++] = data[pos];
> > + frame = ppp_decode(link, link->buffer);
>
> This function along with ppp_decode do almost exactly the same thing as
> gsm0710_advanced_extract_frame in gsm0710.c. They both do HDLC frame
> decoding, and the only difference I can see is in the fcs table. Can we
> combine these somehow?
Possibly - although in theory in addition to the escaping that you do
in the gsm0710 code, we have to support a negotiated accm (which you see
I've not yet implemented here). We also in theory should support PFC and
ACFC (which the one modem I tested with required, otherwise it refused to
ack my Configure-Request). I think there may eventually be enough
differences to keep these separate.
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Kristen,
> On Thu, 11 Mar 2010 20:17:49 -0600
>
> Denis Kenzior wrote:
> > > + } else {
> > > + /* store last flag character */
> > > + link->buffer[link->index++] = data[pos];
> > > + frame = ppp_decode(link, link->buffer);
> >
> > This function along with ppp_decode do almost exactly the same thing as
> > gsm0710_advanced_extract_frame in gsm0710.c. They both do HDLC frame
> > decoding, and the only difference I can see is in the fcs table. Can we
> > combine these somehow?
>
> Possibly - although in theory in addition to the escaping that you do
> in the gsm0710 code, we have to support a negotiated accm (which you see
> I've not yet implemented here). We also in theory should support PFC and
> ACFC (which the one modem I tested with required, otherwise it refused to
> ack my Configure-Request). I think there may eventually be enough
> differences to keep these separate.
>
You will have to explain to me what that all means ;) However, it would be
ideal if we can create a set of utilities that can be shared between ppp and
mux code (maybe with extra configuration parameters turning on/off or passing
in
parameters required for pfc/acfc/accm support.)
No sense writing, testing and debugging (and more importantly maintaining) the
same code twice.
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Kristen,
> This patch implements the basic PPP protocol. LCP, NCP etc. are handled in a
> different patch.
>
> ---
> Makefile.am |4
> gatchat/gatppp.c | 552
> ++
> gatchat/gatppp.h | 59
> gatchat/gatppp_internal.h | 97
> 4 files changed, 711 insertions(+), 1 deletion(-)
so before we merge this patch and then continue development in the oFono
tree, we have some renaming to do.
Denis and I talked about it on IRC. And what we actually want is
something like this:
gatchat/gatppp.hPublic include file
gatchat/gatppp.cImplementation of PPP API
gatchat/ppp.h Internal header file
gatchat/ppp.c Generic PPP stuff (maybe not needed)
gatchat/ppp_.c Protocol specific extensions
This makes it similar to what we do with gsm7010.[ch] and
ringbuffer.[ch]. So this means for internal structures and functions the
prefix should be ppp_*. Similar to what we do with GSM 70.10 and the
ring buffer implementation.
We might have mislead you with parts of the previous review. I apologize
for that. The more we are looking at your patches it becomes clearer
which direction we wanna take.
> +
> +typedef enum _GAthase {
> + DEAD = 0,
> + ESTABLISHMENT,
> + AUTHENTICATION,
> + NETWORK,
> + TERMINATION,
> +} GAthase;
> +
> +typedef enum _GAtPPPEvents {
> + PPP_UP = 1,
> + PPP_OPENED,
> + PPP_SUCCESS,
> + PPP_NONE,
> + PPP_CLOSING,
> + PPP_FAIL,
> + PPP_DOWN
> +} GAtPPPEvents;
If we don't expose them in public headers, then typedefs are not needed.
We actually dislike typedefs. The only reason for having them in public
GAtChat headers is to confirm with GLib style.
> +struct ppp_packet_handler {
> + guint16 proto;
> + void (*handler)(gpointer priv, guint8 *packet);
> + gpointer priv;
> +};
> +
> +#define ppp_info(packet) \
> + (packet+4)
> +
> +#define ppp_proto(packet) \
> + (ntohs(*((guint16*)(packet+2
Arithmetic operations requires spaces. Even in defines. So it packet + 4
etc.
When you cast don't forget whitepaces. So (guint16 *) (packet...
> +struct _GAtPPP {
> + gint ref_count;
> + GAthase phase;
> + guint8 buffer[BUFFERSZ];
> + int index;
> + gint mru;
> + guint16 auth_proto;
> + char user_name[256];
> + char passwd[256];
> + gboolean pfc;
> + gboolean acfc;
> + guint32 xmit_accm[8];
> + guint32 recv_accm;
> + GIOChannel *modem;
> + GQueue *event_queue;
> + GQueue *recv_queue;
> + GAtPPPConnectFunc connect_cb;
> + gpointer connect_data;
> + GAtPPPDisconnectFunc disconnect_cb;
> + gpointer disconnect_data;
> + gint modem_watch;
> +};
> +
> +void __ppp_generate_event(GAtPPP *ppp, GAtPPPEvents event);
> +void __ppp_register_packet_handler(struct ppp_packet_handler *handler);
> +void __ppp_transmit(GAtPPP *ppp, guint8 *packet, guint infolen);
> +void __ppp_set_auth(GAtPPP *ppp, guint8 *auth_data);
> +void __ppp_set_recv_accm(GAtPPP *ppp, guint32 accm);
> +guint32 __ppp_get_xmit_accm(GAtPPP *ppp);
> +void __ppp_set_pfc(GAtPPP *ppp, gboolean pfc);
> +gboolean __ppp_get_pfc(GAtPPP *ppp);
> +void __ppp_set_acfc(GAtPPP *ppp, gboolean acfc);
> +gboolean __ppp_get_acfc(GAtPPP *ppp);
Just use ppp_* here and forget about __ppp_*. I will add autofoo magic
to ensure we are not accidentally exporting these. It is not your
concern.
> + * PPP library with GLib integration
> + *
> + * Copyright (C) 2010 Intel Corporation. All rights reserved.
Nitpick here. Copyright is 2009-2010. Applies to all new files you are
adding.
> + /*
> + * do the octet stuffing. Add 2 bytes to the infolen to
> + * include the protocol field.
> + */
> + frame = ppp_encode(ppp, packet, infolen+2, &framelen);
> + if (!frame) {
> + g_printerr("Failed to encode packet to transmit\n");
> + return;
> + }
> +
> + /* transmit through the lower layer interface */
> + /*
> + * TBD - should we just put this on a queue and transmit when
> + * we won't block, or allow ourselves to block here?
> + */
> + status = g_io_channel_write_chars(ppp->modem, (gchar *) frame,
> + framelen, &bytes_written, &error);
> +
> + g_free(frame);
> +
> +}
Look out for unneeded empty lines at the end of functions. We don't want
them.
> +static void ppp_transition_phase(GAtPPP *ppp, guint phase)
> +{
> + /* don't do anything if we're already there */
> + if (ppp->phase == phase)
> + return;
> +
> + /* set new phase */
> + ppp->phase = phase;
> +
> + switch(phase) {
> + case ESTABLISHMENT:
> + ppp_ppp_establishment(ppp);
> + break;
> + case AUTHENTICATION:
> + ppp_authenticate(ppp);
> + break;
> + case TERMINATION:
> +
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Kristen, > This patch implements the basic PPP protocol. LCP, NCP etc. are handled in a > different patch. > > --- > Makefile.am |4 > gatchat/gatppp.c | 133 > gatchat/gatppp.h | 59 +++ > gatchat/ppp.c| 455 > +++ > gatchat/ppp.h| 130 +++ > 5 files changed, 780 insertions(+), 1 deletion(-) please keep the patches whitespace clean. Otherwise you will make it really hard and complicated for everybody if we have to do whitespace cleanup in between patches. Applying: Add PPP protocol support with HDLC framing /data/devel/ofono/.git/rebase-apply/patch:572: space before tab in indent. if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN) /data/devel/ofono/.git/rebase-apply/patch:573: space before tab in indent. return FALSE; fatal: 2 lines add whitespace errors. Using an editor that visualizes whitespaces and tabs might help here. And empty lines at the end of a file. Please remove them and if your editor is too stupid, then please pick a different editor. Applying: Add PPP protocol support with HDLC framing /data/devel/ofono/.git/rebase-apply/patch:166: new blank line at EOF. + /data/devel/ofono/.git/rebase-apply/patch:230: new blank line at EOF. + fatal: 2 lines add whitespace errors. Before you submit any patches, you might wanna quickly check with git am that they still apply. My .gitconfig uses a strict apply policy: [diff] color = auto check = true [apply] whitespace = error [format-patch] check = true Having diff and format-patch color your whitespace damages is always a good idea. Then you see what is wrong. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
