[En-Nut-Discussion] Status of Point-To-Point protocol over RS-232
Nathan Moore
nategoose at gmail.com
Sun Apr 17 02:40:29 CEST 2011
Hey Harald,
A year or two ago I did some work adding support for another network
device that also used a serial port and AHDLC
to a custom Nut/OS version for the company I worked for at the time.
Most of this would be of little interest to you,
but there was some AHDLC changes for AVR that may be of general
interest. There were also a few changes I made
to the PPP code to take advantage of these AHDLC changes and to
account for things I'd learned in my own device
driver. With these changes I was able to get a good bit closer to the
maximum upload bandwidth for PPP with the
modem I tested with.
Since I only cared about AVR I didn't touch the code for the other
architectures.
My primary goal in the changes I made were small and fast code for AVR
and fastest performance I could get with slow
serial connections, and I only worried about compiling with GCC.
I don't currently have any AVR development hardware or development
environment set up, but would like to pass along
the code in case someone would like to try to get these ready for
inclusion in Nut.
It's also possible that I've left something out of this by accident.
I had to cherry pick the changes that I made because
some of them were to support custom hardware and others weren't of
general interest, but I may have missed something.
== what I did ==
In nut/arch/avr/ahdlcavr.c I changed the IN_ACC_MAP macro function to
call an inline function (because I needed
temporary local variables) that treated the ACC MAP bitmask as
separate bytes rather than a 4 byte variable. Since
AVR only has to shift by one bit at a time (at least last time I
looked) the old code would have needed to shift anywhere
from 0 to 31 times, but this code only has to shift from 0 to 8 times
because it only examines the 1 byte out of 4
in the map that corresponds to the value being tested for.
As the rest of the code was I could have just hardcoded the IN_ACC_MAP
macro to escape everything between
0 and 31 because the ACCM was never being set by PPP's LCP layer, but
I made that happen too. That was
complicated because it had to be done within the state machine at the
correct places. I think that I got that right.
It's possible that I screwed up the endian on that, though. I only
needed mask all and mask none, which wouldn't
have had issues with an endian mistake.
Additionally I tried to make as many functions as I could "static
inline" to encourage GCC to generate better code.
It's been a long time since I compiled or ran this code, and I just
took a few minutes to try to port it from 4.4.0 to
4.8.9 so that someone might be able to cleanly apply the patch and see
what they think of it.
== what I didn't do ==
There is a lot of uart0 v/s uart1 code in ahdlcavr.c file that could
have gone into the ahdlc0.c and ahdlc1.c files.
I wanted to move it, but it was never a priority. Looking at that
code again I wanted to make those changes
again, but since I can't test it I will just have to resist that urge.
I wanted to make some of the other configurable things in PPP actually
configurable, but never did.
I also looked into some header compression stuff, but never got around to it.
== unshared stuff ==
In order to get some stuff working I had to change some stuff about
NutNetIfConfig so that each interface
type (ethernet, PPP, other) had a callback function within its
interface control block (ICB) that handled
the device specific stuff, and made a function for each of these devices.
This allowed me to more easily add another network device type and
also allowed me to keep PPP and other
network device types from writing stuff to the EEPROM that 1) didn't
need to be written and 2) competed
with the ethernet device writing to the same area (I think that this
behavior in Nut may have changed
by now). I still think that this is a better model since it keeps PPP
code from being present if the user
only uses ethernet and vica versa.
It also allowed me an opportunity to kick off some other operations
in my custom device.
========== the patch =============
diff -r 972b754993cc arch/avr/dev/ahdlcavr.c
--- a/arch/avr/dev/ahdlcavr.c Sat Apr 16 17:31:21 2011 -0400
+++ b/arch/avr/dev/ahdlcavr.c Sat Apr 16 19:58:49 2011 -0400
@@ -153,7 +153,33 @@
/*!
* Checks the 32-bit ACCM to see if the byte needs un-escaping
*/
-#define IN_ACC_MAP(c, m) (( ((u_char) (c)) < 0x20) && ((m) & (1UL <<
(c))) != 0)
+#define IN_ACC_MAP(c, m) in_acc_map(c, &(m))
+/* Trust me, this is a whole lot smaller when compiled than it looks in C.
+ * It is written explicitly so that gcc can make good AVR asm out of it. */
+static inline uint8_t in_acc_map(u_char ch, void * esc_mask)
__attribute__((always_inline)); /* gcc specific attribute */
+static inline uint8_t in_acc_map(u_char ch, void * esc_mask)
+{
+ const uint8_t shift_mask = 0x07;
+ const uint8_t index_mask = 0x18;
+ const uint8_t over_mask = ~(shift_mask|index_mask);
+
+ uint8_t shift, index, emask;
+ uint8_t * emskp = esc_mask;
+
+ if (over_mask & ch) {
+ return 0;
+ }
+
+ shift = shift_mask & ch;
+
+ index = ch >> (uint8_t)3;
+
+ /* NOTE: This assumes that the esc bit field is little endian,
+ * which it should have been switched to before being set. */
+
+ emask = emskp[index];
+ return emask & ( ((uint8_t)1) << shift );
+}
#ifndef NUT_THREAD_AHDLCRXSTACK
#define NUT_THREAD_AHDLCRXSTACK 512
@@ -769,6 +795,8 @@
* - UART_GETRAWMODE, conf points to an uint32_t value
receiving 0 (off) or 1 (on).
* - HDLC_SETIFNET, conf points to a pointer containing
the address of the network device's NUTDEVICE structure.
* - HDLC_GETIFNET, conf points to a pointer receiving
the address of the network device's NUTDEVICE structure.
+ * - HDLC_SETTXACCM, conf points to an uint32_t value
containing the ACC Map in host endian.
+ * - HDLC_GETTXACCM, conf points to an uint32_t value
receiving the ACC Map in host endian.
*
* \param conf Points to a buffer that contains any data required for
* the given control function or receives data from that
@@ -1007,6 +1035,16 @@
*ppv = dev->dev_icb;
break;
+ /* AVR host endian is little endian, and the ACCM should have been switched
+ * to host endian when the value was read in. */
+ case HDLC_SETTXACCM:
+ dcb->dcb_tx_accm = (*lvp)
+ break;
+
+ case HDLC_GETTXACCM:
+ (*lvp) = dcv->dcb_tx_accm;
+ break;
+
default:
rc = -1;
break;
@@ -1039,8 +1077,8 @@
dcb = dev->dev_dcb;
memset(dcb, 0, sizeof(AHDLCDCB));
dcb->dcb_base = dev->dev_base;
- dcb->dcb_rx_buf = NutHeapAlloc(256);
- dcb->dcb_tx_buf = NutHeapAlloc(256);
+ dcb->dcb_rx_buf = NutHeapAlloc(256 * 2);
+ dcb->dcb_tx_buf = dcb->dcb_rx_buf + 256;
dcb->dcb_rx_mru = 1500;
dcb->dcb_tx_mru = 1500;
dcb->dcb_tx_accm = 0xFFFFFFFF;
@@ -1133,8 +1171,6 @@
/* We failed, clean up. */
if (dcb->dcb_rx_buf)
NutHeapFree((void *) dcb->dcb_rx_buf);
- if (dcb->dcb_tx_buf)
- NutHeapFree((void *) dcb->dcb_tx_buf);
return -1;
}
diff -r 972b754993cc include/dev/uart.h
--- a/include/dev/uart.h Sat Apr 16 17:31:21 2011 -0400
+++ b/include/dev/uart.h Sat Apr 16 19:58:49 2011 -0400
@@ -369,6 +369,20 @@
*/
#define UART_GETRAWMODE 0x012b
+/*! \brief AHDLC _ioctl() command code to set the ACCM (Control
Character Mask).
+ *
+ * During the LCP stage of PPP negotiation both peers inform each other which
+ * of the control characters (0-31) will not require escaping when being
+ * transmitted. This allows the PPP layer to tell the HDLC layer about this
+ * so that data may be transmitted quicker (no escapes).
+ */
+#define HDLC_SETTXACCM 0x012c
+
+/*! \brief AHDLC _ioctl() command code to get the ACCM (Control
Character Mask).
+ *
+ * Just in case someone ever wants to see what it currently is.
+ */
+#define HDLC_GETTXACCM 0x012d
/*!
* \addtogroup xgUARTStatus
* \brief UART device status flags,
diff -r 972b754993cc net/lcpin.c
--- a/net/lcpin.c Sat Apr 16 17:31:21 2011 -0400
+++ b/net/lcpin.c Sat Apr 16 19:58:49 2011 -0400
@@ -86,6 +86,8 @@
#include <netinet/ppp_fsm.h>
#include <netinet/in.h>
#include <string.h>
+#include <dev/usart.h>
+#include <io.h>
/*!
* \addtogroup xgLCP
@@ -98,7 +100,7 @@
/*
* Received Configure-Request.
*/
-static void LcpRxConfReq(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
+static inline void LcpRxConfReq(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
{
PPPDCB *dcb = dev->dev_dcb;
int rc = XCP_CONFACK;
@@ -264,6 +266,7 @@
if (rc == XCP_CONFACK) {
if (dcb->dcb_lcp_state == PPPS_ACKRCVD) {
dcb->dcb_lcp_state = PPPS_OPENED;
+ _ioctl(dcb->dcb_fd, HDLC_SETTXACCM, &(dcb->dcb_accm) );
if (dcb->dcb_auth == PPP_PAP)
PapTxAuthReq(dev, ++dcb->dcb_reqid);
else
@@ -279,7 +282,7 @@
* Configure-Ack received.
* Never called in INITIAL or STARTING phase.
*/
-static void LcpRxConfAck(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
+static inline void LcpRxConfAck(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
{
PPPDCB *dcb = dev->dev_dcb;
XCPOPT *xcpo;
@@ -360,6 +363,7 @@
* ACK sent and ACK received.
*/
dcb->dcb_lcp_state = PPPS_OPENED;
+ _ioctl(dcb->dcb_fd, HDLC_SETTXACCM, &(dcb->dcb_accm) );
if (dcb->dcb_auth == PPP_PAP)
PapTxAuthReq(dev, ++dcb->dcb_reqid);
@@ -381,7 +385,7 @@
/*
* Configure-Nak or Configure-Reject received.
*/
-static void LcpRxConfNakRej(NUTDEVICE * dev, uint8_t id, NETBUF * nb,
uint8_t rejected)
+static inline void LcpRxConfNakRej(NUTDEVICE * dev, uint8_t id,
NETBUF * nb, uint8_t rejected)
{
PPPDCB *dcb = dev->dev_dcb;
@@ -445,7 +449,7 @@
/*
* Terminate-Request received.
*/
-static void LcpRxTermReq(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
+static inline void LcpRxTermReq(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
{
PPPDCB *dcb = dev->dev_dcb;
@@ -468,7 +472,7 @@
/*
* Terminate-Ack received.
*/
-static void LcpRxTermAck(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
+static inline void LcpRxTermAck(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
{
PPPDCB *dcb = dev->dev_dcb;
@@ -527,7 +531,7 @@
/*
* Received a Code-Reject.
*/
-static void LcpRxCodeRej(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
+static inline void LcpRxCodeRej(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
{
PPPDCB *dcb = dev->dev_dcb;
@@ -540,7 +544,7 @@
/*
* Received an Echo-Request.
*/
-static void LcpRxEchoReq(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
+static inline void LcpRxEchoReq(NUTDEVICE * dev, uint8_t id, NETBUF * nb)
{
PPPDCB *dcb = dev->dev_dcb;
diff -r 972b754993cc net/lcpout.c
--- a/net/lcpout.c Sat Apr 16 17:31:21 2011 -0400
+++ b/net/lcpout.c Sat Apr 16 19:58:49 2011 -0400
@@ -89,6 +89,8 @@
#include <netinet/if_ppp.h>
#include <netinet/ppp_fsm.h>
#include <net/ppp.h>
+#include <dev/usart.h>
+#include <io.h>
/*!
* \addtogroup xgLCP
@@ -131,7 +133,7 @@
return 0;
}
-void LcpResetOptions(NUTDEVICE * dev)
+static inline void LcpResetOptions(NUTDEVICE * dev)
{
PPPDCB *dcb = dev->dev_dcb;
@@ -140,8 +142,10 @@
dcb->dcb_neg_magic = new_magic;
dcb->dcb_loc_magic = 0;
dcb->dcb_rem_magic = 0;
- dcb->dcb_accm = LCP_DEFOPT_ASYNCMAP;
+ dcb->dcb_accm = 0xFFffFFff;
dcb->dcb_loc_mru = 1500;
+
+ _ioctl(dcb->dcb_fd, HDLC_SETTXACCM, &dcb->dcb_accm );
}
/*
@@ -170,7 +174,7 @@
xcpo = nb->nb_ap.vp;
xcpo->xcpo_type = LCP_ASYNCMAP;
xcpo->xcpo_len = 6;
- xcpo->xcpo_.ul = htonl(LCP_DEFOPT_ASYNCMAP);
+ xcpo->xcpo_.ul = htonl(LCP_DEFOPT_ASYNCMAP); /* Should this
be "= 0;" instead? */
/*
* This is a temporary hack. In the initial version
More information about the En-Nut-Discussion
mailing list