Eric Yeo wrote:
On Sun, 2002-07-14 at 00:27, Oded Arbel wrote:
  
Got all that - but still I think the logic you imploy is not correct: 
since CRTSCTS is already being set for the modem device, its kind of 
silly to set it and then disable it again - instead, simply enable it if 
needed (disable-crtscts not being set). I submit exhibit A - smsc_at2 
disable-crtscts patch, the Oded way ;-)

-- 
Oded Arbel
m-Wise mobile solutions


----

    

  
--- gw/smsc_at2.c	2002-06-06 17:09:24.000000000 +0000
+++ gw/smsc_at2.c	2002-07-13 14:22:47.000000000 +0000
@@ -79,7 +79,8 @@
     tios.c_oflag &= ~ONLCR; /* no NL to CR-NL mapping outgoing */
     tios.c_iflag |= IGNPAR; /* ignore parity */
     tios.c_iflag &= ~INPCK;
-    tios.c_cflag |= CRTSCTS; /* enable hardware flow control */
+    if (privdata->modem->use_crtscts)
+	tios.c_cflag |= CRTSCTS; /* enable hardware flow control */
     tios.c_cc[VSUSP] = 0; /* otherwhise we can not send CTRL Z */
 
     /*
@@ -2053,6 +2131,9 @@
         modem->message_storage = cfg_get(grp, octstr_imm("message-storage"));
 
         cfg_get_bool(&modem->enable_mms, grp, octstr_imm("enable-mms"));
+	
+	cfg_get_bool(&modem->use_crtscts, grp, octstr_imm("disable-crtscts"));
+	modem->use_crtscts = !modem->use_crtscts;
 
         /*	
         if (modem->message_storage == NULL)
    

On second thought, let me be more specific in pointing out the problem
with your code.

The function "at2_open_device" sets the terminal flags by first reading
the existing settings. Then it turns certain bits on or off depending on
what it requires. If I wish to turn CRTSCTS off, it may not work because
according to the logic of your code, you only turn it on if you require
it. What happens if CRTSCTS is on to start with? There is nothing to
turn it off when I want it off!

My original patch works. Perhaps the only change I could make is this:

*** smsc_at2.c  Sat Jul 13 22:01:58 2002
--- smsc_at2.c.new      Sat Jul 13 22:02:48 2002
***************
*** 79,90 ****
      tios.c_oflag &= ~ONLCR; /* no NL to CR-NL mapping outgoing */
      tios.c_iflag |= IGNPAR; /* ignore parity */
      tios.c_iflag &= ~INPCK;
-     tios.c_cflag |= CRTSCTS; /* enable hardware flow control */
      tios.c_cc[VSUSP] = 0; /* otherwhise we can not send CTRL Z */
  
      if (privdata->modem->disable_crtscts) {
          tios.c_cflag &= ~CRTSCTS; /* disable hardware flow control */
      }
  
      /*
      if ( ModemTypes[privdata->modemid].enable_parity )
--- 79,92 ----
      tios.c_oflag &= ~ONLCR; /* no NL to CR-NL mapping outgoing */
      tios.c_iflag |= IGNPAR; /* ignore parity */
      tios.c_iflag &= ~INPCK;
      tios.c_cc[VSUSP] = 0; /* otherwhise we can not send CTRL Z */
  
      if (privdata->modem->disable_crtscts) {
          tios.c_cflag &= ~CRTSCTS; /* disable hardware flow control */
      }
+     else {
+         tios.c_cflag |= CRTSCTS; /* enable hardware flow control */
+     }
  
      /*
      if ( ModemTypes[privdata->modemid].enable_parity )

But that is fairly minor and does not affect performance. This is really
nit-picking. I will say no more of this.
  
No - you are correct. I should have explicitly disabled the CRTSCTS flag. OTOH, this code is more concise and clear then the patch you originaly submited (IMHO), as it takes less directives and is more clear on what is done. specificly, it does not set the CRTSCTS flag twice as you have done.  of course only one change is necessary. in addition to adding the "disable-crtscts" directive to the cfg.def as you have done, an additional member must be added to the AT2 header file, which will simply be called use_crtscts instead of disable_crtscts. a more complete patch (with your correction) is attached.

--
Oded Arbel
m-Wise mobile solutions

--- kannel-dev/gateway/gw/smsc_at2.c    2002-06-06 17:09:24.000000000 +0000
+++ gateway/gw/smsc_at2.c       2002-07-15 12:53:27.000000000 +0000
@@ -79,7 +79,10 @@
     tios.c_oflag &= ~ONLCR; /* no NL to CR-NL mapping outgoing */
     tios.c_iflag |= IGNPAR; /* ignore parity */
     tios.c_iflag &= ~INPCK;
-    tios.c_cflag |= CRTSCTS; /* enable hardware flow control */
+    if (privdata->modem->use_crtscts)
+       tios.c_cflag |= CRTSCTS; /* enable hardware flow control */
+    else 
+       tios.c_cflag &= ~CRTSCTS; /* disable hardware flow control */
     tios.c_cc[VSUSP] = 0; /* otherwhise we can not send CTRL Z */
 
     /*
@@ -2053,6 +2133,9 @@
         modem->message_storage = cfg_get(grp, octstr_imm("message-storage"));
 
         cfg_get_bool(&modem->enable_mms, grp, octstr_imm("enable-mms"));
+       
+       cfg_get_bool(&modem->use_crtscts, grp, octstr_imm("disable-crtscts"));
+       modem->use_crtscts = !modem->use_crtscts;
 
         /*     
         if (modem->message_storage == NULL)
--- kannel-dev/gateway/gw/smsc_at2.h    2002-06-06 17:09:25.000000000 +0000
+++ gateway/gw/smsc_at2.h       2002-07-13 14:22:05.000000000 +0000
@@ -54,6 +54,7 @@
     int        broken;
     Octstr *message_storage;
     int        enable_mms;
+    int use_crtscts;
 } ModemDef;
 
 typedef struct PrivAT2data {
--- kannel-dev/gateway/gwlib/cfg.def    2002-07-12 17:51:09.000000000 +0000
+++ gateway/gwlib/cfg.def       2002-07-15 12:58:43.000000000 +0000
@@ -335,6 +345,7 @@
     OCTSTR(broken)
     OCTSTR(message-storage)
     OCTSTR(enable-mms)
+    OCTSTR(disable-crtscts)
 )
 
 MULTI_GROUP(mysql-connection,

Reply via email to