Email: Password: Remember Me | Create Account (Free)

Back to Subject List

Old thread has been locked -- no new posts accepted in this thread
Jan Waclawek
02/28/06 05:18
Read: 885 times

#110913 - comments
Responding to: Jan Waclawek's previous message

I promised to comment on your interrupt-driven serial program and I will also comment on mine later if time permits.

My comments in red.

Of course, I am nothing of perfect etc. so these are only my ideas. Arguable, of course, all of them.

Jan Waclawek

CR            EQU    0DH 
LF            EQU    0AH 
TXBUFSZ      EQU    2             ;Define buffer sizes 
2 bytes long Tx buffer... Not really worth, is 
it? See also comment on TXBUFSZ in the interrupt routine itself.
RXBUFSZ      EQU    8 
              ;                    ;Indexes 
RX_IN         EQU    08H           ;RX next to go in 
RX_OUT       EQU    09H           ;RX next to go out 
TX_IN         EQU    0AH           ;TX next to go in 
TX_OUT       EQU    0BH           ;TX next to go out 
RXBUF         EQU    0CH           ;Allow RXBUFSZ to next variable 
TXBUF         EQU    14H           ;Allow TXBUFSZ to next variable 
the definition of variables using EQU is not 
the best idea; if you need to squeeze in some other variable or 
change the sizes of buffers, it would require a lot of work. It 
is better to use DB or similar pseudoinstructions intended to 
reserve space for variables; for buffers (with already defined 
size) use DB TXBUFSZ .
CHR           EQU    16H 
End of variables in RAM -> the initial value of
 stack should come here (see MOV SP,#17h below; should be like 
MOV SP,#SPINIT and SPINIT definition should come here). Again, 
the idea is to have as much job done by the assembler itself 
automatically, when juggling with the variables. 

NEEDTI       BIT    01H           ;Clear it if TI=1 is needed 
              ;                    ;Set if TI=1 is not needed 
       ORG    0h 
       LJMP   MAIN                 ;Reset vector 
       ORG    23h  
       LJMP   SER_ISR             ;Serial port interrupt vector 
       ORG    40h 
MAIN:  MOV    SP,#17h             ;Initialize stack pointer 
see comment on initial value of stack ponter above
       LCALL  SERINIT             ;Initialize serial port 
       SETB   EA                   ;Enable all interrupts 
       MOV    DPTR,#SINIT         ;Serial Port Initialized! 
       LCALL  TEXT_OUT            ;Send the message out 
LOOP:                              ;Repeating loop 
       MOV    A,R1 this
       CPL    A    and this
       JZ     LOOP and this
can be written as one instruction: CJNE R1,#0FFh,LOOP
Anyway; IMHO it is not the best idea to indicate a valid 
received character in this way - see comment below in GETCHR routine
       AJMP   LOOP                 ;Loop back 
It would be nice to have a comment here, like: "the program here waits
for any received character and echoes it back". But, is this a good example for 
interrupt-driven serial communication? It can be done in the same way
also using polling... Some other, more complicated, example would be fine.

A comment should come here, saying something like: 
"this is a string output routine, something like PRINT in BASIC or 
"printf" in C or "write" in Pascal. Sends out the string pointed
to by DPTR, terminated by zero.
       CLR    A                    ;dptr has message 
       MOVC   A,@A+DPTR           ;get the next character 
       INC    DPTR                 ;move index to next position 
       JZ     WT2                  ;if zero then end of message 
       MOV    R1,A                 ;else put the chr in the tx buffer 
Now THIS (a delay loop) has nothing to do here! 
It indicates some FATAL problem with interrupt-driven communication. The point 
of using interrupts was not to delay the "main" program by the serial routines, 
but this is contradicting it.
Correctly, the  transmitting routine should check for empty space in the 
transmit buffer and if available, put the character otherwise wait. There is a 
buffer empty space check in PUTCHR, but it only fails to put the character and 
THIS routine is suposed to check whether it failed and retry.
In this way, if the string is shorter than the transmit buffer, it would be
placed into the buffer immediately without any delay, and the main program could
move on while the buffer would be transmitted "on the background" by the interrupt
routine - and that's the whole point of interrupt driven communication.
       mov    r3,#2                ;Temporary delay loop 
LP1:   mov    r2,#225             ;to prevent buffer overrun 
       djnz   r2,$ 
       djnz   r3,LP1 
       AJMP   TEXT_OUT 
A note on the preferencial use of particular form of JMP: 
if the jump is explicitly given (not derived by assembler from the genericv JMP),
in short self-contained routines such as this the SJMP is preferred rather than
AJMP (if the length of jump permits it of course). The reason is, if this routine
gets shifted somewhere on the 2k page boundary, AJMP would fail 
while SJMP would not.
WT2:   RET 
;                                  ;R1 has character 
       SETB   C                    ;using C to store state of EA 
       JBC    EA,PCSKP            ;if interrupts enabled, then disable 
       CLR    C                    ;EA was already disabled 
Ehm... what about... MOV C,EA ?
PCSKP:       PUSH   PSW           ;store current status, including EA 
... but not MOV C,EA is needed; why not PUSH IE ?
       MOV    CHR,R1              ;store the character 
there is NO NEED to store the character in CHR - R1 does not
get corrupted until the character is really stored in the buffer!
       CLR    C 
       MOV    A,TX_IN 
       SUBB   A,TX_OUT 
INCORRECT! This code attempts to get the occupied space in buffer
and compare it against the buffer size (below) to get the empty space.
However, it completely misses the fact that in circular buffer the "head" pointer
(TX_IN) may get BELOW the "tail" pointer (TX_OUT)! Although this implementation 
of circular buffer is - ehm - nonstandard (the pointers are NOT the actual
pointers to the buffer - see below) and the described problem will occur less
frequently than in the "normal" implementation (it takes 256 bytes to transmit for 
the "wraparound" of the TX_IN/TX_OUT to occur), it is still not a proper
way to go.
       CLR    C 
       SUBB   A,#TXBUFSZ          ;TXBUFSZ = 2 
       JC     TBUFOK              ; 
       MOV    R1,#0FFh            ;else error RETURN VALUE -1 
       AJMP   PCXIT                ;buffer full  return 
       MOV    A,TX_IN 
       ANL    A,#TXBUFSZ-1       ;TXBUFSZ =2-1=1 0 through 1 = 2 
Now this is a method which I object.
The pointers don't properly wrap around at the end of buffer; rather, 
their topmost bits are masked out when calculating the real address.
It assumes that the buffer sizes are in power of two (should be added as a note
at the buffer length declaration).
It also prevents to use the real addresses as pointers, so they the address
has to be calculated each time.
Maybe it works, but I don't like it. I could imagine working with something 
similar, but not in this form - I would keep the PROPER address as pointer,
and I would mask it after incrementing - it requires exact positioning
of the buffer but I think this is more appropriate than this approach.
       ADD    A,#TXBUF            ;# of characters in buffer+buffer address 
       MOV    R0,A                 ;location in buffer 
       MOV    @R0,CHR             ;put character in buffer 
       INC    TX_IN                ;move index 
       JNB    NEEDTI,SKPTI       ;exit if no more characters in buffer 
       CLR    NEEDTI              ;else more to send 
       SETB   TI                   ;interrupt and send them 
       MOV    R1,#00h             ;done - return value 0 
       POP    PSW                  ;restore and return 
       MOV    EA,C 
       SETB   C                    ;use C to store the state of EA 
       JBC    EA,GCSKP            ;if all enabled then disable 
       CLR    C                    ;all were disabled already 
       PUSH   PSW                  ;store the current state 
The same applies for storing EA as above...
       CLR    C 
       MOV    A,RX_IN             ;how many characters in buffer? 
       SUBB   A,RX_OUT 
       JNZ    RBUFNZ              ;jump if buffer not empty 
the four above lines merge into 
       MOV    R1,#0FFh            ;else buffer is empty return value -1 
       AJMP   GCXIT                ;exit with buffer empty code 
       MOV    R1,RX_OUT           ;next to get out 
       INC    RX_OUT              ;move index 
       MOV    A,R1 
There is no need to move RX_OUT into R1 first, 
then move it from R1 into A
       ANL    A,#RXBUFSZ-1       ;RXBUFSZ = 8-1 =7 0 through 7 = 8 
       ADD    A,#RXBUF            ;# of characters in buffer+buffer address 
       MOV    R0,A                 ;R0 has buffer location 
       MOV    A,@R0                ;get chr from buffer 
       MOV    R1,A                 ;put chr in R1 
       POP    PSW                  ;restore previous state 
       MOV    EA,C                 ;restore previous interrupt state 
       RET                         ;return 
SERINIT:                          ;Initialize serial port 
       CLR    A 
       MOV    TX_IN,A             ;initialize indexes 
       MOV    TX_OUT,A 
       MOV    RX_IN,A 
       MOV    RX_OUT,A 
       CLR    TR1 
       CLR    TI 
       CLR    RI 
                                   ;MODE 1 REN 
       MOV    TMOD,#00100001B    ;TIMER 1 MODE 8 BIT AUTO RELOAD 
        MOV    TH1,#0FDh           ;TIMER RELOAD VALUE 
       SETB   TR1                  ;START TIMER 
       SETB   NEEDTI 
       SETB   ES 
       DB     CR,LF 
       DB     'Serial Port Initialized! ' 
CRLF:  DB     CR,LF,0 
       PUSH   ACC                  ;store current state 
       PUSH   PSW 
       MOV    PSW,#00h 
       PUSH   00h 
I think this deserves a comment - this is storing R0 from 
register bank 0, and MOV PSW,#0 ensures, that the isr will really work with
register bank 0...

       JNB    RI,TXISR            ;if not receive then check xmit 
       CLR    RI                   ;else clear RI and process 
       CLR    C 
       MOV    A,RX_IN 
       SUBB   A,RX_OUT            ;how many characters in buffer? 
       ANL    A,#0-RXBUFSZ       ; 
Argh... Why this mask??? No need even for that twisted pointer
       JNZ    TXISR                ;jump if no characters in buffer 
FATAL! The receiver refuses to receive further characters if 
there is already one character in buffer - they got lost!!!
       MOV    A,RX_IN             ;else  
       ANL    A,#RXBUFSZ-1       ;number of characters in buffer 
       ADD    A,#RXBUF            ;plus buffer address = 
       MOV    R0,A                 ;buffer position 
       MOV    @R0,SBUF            ;put received character in buffer 
       INC    RX_IN                ;increment buffer position 
       JNB    TI,ISRXIT           ;if TI=0 then exit 
       CLR    TI                   ;else clear TI and process 
       MOV    A,TX_IN             ;how many characters in buffer? 
       XRL    A,TX_OUT 
       JZ     TXBUFZ              ;jump if buffer empty 
MOV A,TX_OUT; CJNE A,TX_IN,TXBUFZ and the following line 
can be omitted
       MOV    A,TX_OUT            ;else 
       ANL    A,#TXBUFSZ-1       ;number of characters in buffer 
       ADD    A,#TXBUF            ;plus buffer address = 
       MOV    R0,A                 ;buffer position 
       MOV    A,@R0                ;get character from buffer 
       MOV    SBUF,A              ;send character out 
       INC    TX_OUT              ;increment buffer position 
       CLR    NEEDTI              ;need to set TI 
       AJMP   ISRXIT              ;exit ISR 
       SETB   NEEDTI              ;buffer was empty dont need to set TI 
       POP    00h                  ;restore and return 
       POP    PSW 
       POP    ACC 

List of 46 messages in thread
Feedback needed        Jon Ledbetter      12/12/05 13:29      
   Couple of ideas      Sasha Jevtic      12/12/05 14:24      
   Missing      Andy Neil      12/12/05 15:28      
      the source...      Jan Waclawek      12/12/05 16:10      
         Stupid EIA      Andy Neil      12/13/05 00:52      
      Maybe a Name change?      Jon Ledbetter      12/12/05 22:30      
         minor but annoying ...      Richard Erlacher      03/17/06 09:33      
   USB      Ian Bell      12/12/05 16:06      
      Limited Experience      Jon Ledbetter      12/12/05 22:43      
      known bad USB/serial      Jan Waclawek      12/13/05 01:56      
      FYI - Targus PA088      Jon Ledbetter      12/13/05 09:46      
   thoughts      Jan Waclawek      12/12/05 16:17      
      I am so stupid      Jan Waclawek      12/13/05 02:02      
         Kickstart      Jon Ledbetter      12/13/05 07:36      
   attribution      Steve M. Taylor      12/13/05 03:41      
   The ONE thing I always have to look up a      Erik Malund      12/13/05 08:03      
   Ok - Second revision, but still working,      Jon Ledbetter      12/15/05 14:16      
      Nice      Steve M. Taylor      12/15/05 15:12      
         OK      Jon Ledbetter      12/15/05 15:33      
         MAX202 vs. MAX232A      Jan Waclawek      12/16/05 02:32      
         or the 232A      Erik Malund      12/16/05 06:32      
      Polled; Interrupt      Andy Neil      12/16/05 00:39      
         polled tx      Jon Ledbetter      12/16/05 07:48      
            Assorted small ideas      Sasha Jevtic      12/17/05 00:19      
               Not a good idea      Kai Klaas      12/17/05 06:46      
                  Serial speeds?      Sasha Jevtic      12/17/05 08:41      
                     Enhanced specifications      Kai Klaas      12/18/05 06:38      
                        That's a strange spec      Sasha Jevtic      12/20/05 15:04      
      formal stuff      Jan Waclawek      12/16/05 02:59      
         formal      Jon Ledbetter      12/16/05 08:23      
            who's the intended audience      Jan Waclawek      12/16/05 09:15      
   3rd Revision      Jon Ledbetter      12/27/05 08:47      
      Busy?      Jon Ledbetter      01/03/06 12:23      
         more than a couple of comments...      Jan Waclawek      01/04/06 16:27      
            So.....      Jon Ledbetter      01/05/06 15:01      
               what I like or not...      Jan Waclawek      01/06/06 08:25      
                  comments      Jan Waclawek      02/28/06 05:18      
                     Comments on Comments      Jon Ledbetter      02/28/06 09:37      
                        bah...      Jan Waclawek      02/28/06 11:25      
                           more problems      Jan Waclawek      02/28/06 13:20      
                              RE: Problems      Jon Ledbetter      02/28/06 13:36      
                              Something Strange      Jon Ledbetter      02/28/06 13:57      
                                 Nope....      Jon Ledbetter      02/28/06 14:28      
                                    interrupt      Jan Waclawek      02/28/06 15:30      
   All members will enjoy      Ralph Sack      03/16/06 21:55      
   Intel serial intro app note      Sasha Jevtic      03/28/06 15:16      

Back to Subject List