more comments

diff --git a/cebpic/nmra-stream.asm b/cebpic/nmra-stream.asm
index 00e20057a7a1d6d69522ff255b84a953ac7ed050..97716aa5a46a06cf45247b5f36ed35493bf97a6d 100644 (file)
--- a/cebpic/nmra-stream.asm
+++ b/cebpic/nmra-stream.asm
@@ -289,6 +289,10 @@ read_from_buffer
        movwf   FSR1H,0                 ; set high byte of IND1 pointer
        movff   INDF1,W,0
        andwf   TOTRACKBIT,0,0          ; mask out bit to be transmitted
+;*** to `mask out' means to clear, eg
+;*** if you were to `mask out bit 0 of 0xff' you'd get 0xfe
+;*** I suggest `test bit to be transmitted' or `check bit ...' or maybe
+;*** `select bit ...'  -iwj
 
        bz      zero_bit_to_track 
        bra     one_bit_to_track
@@ -306,10 +310,12 @@ one_bit_to_track
 
 
 advance_bit
-; rotate tranmitbit to next position 
+; rotate transmitbit to next position 
 
        rrncf   TOTRACKBIT,1,0          ; rotate mask right
-
+;*** surely rrnc (`rotate right not through carry' I assume)
+;*** will leave a copy of the top bit in the N flag ?  Then you
+;*** can use branch if negative.  -iwj
        btfss   TOTRACKBIT,7,0
        call    advance_pointer
 
@@ -327,6 +333,7 @@ advance_pointer
        movwf   FSR1H,0         
        movff   INDF1,W,0
        andlw   0x80
+;*** bit test bit 7 of INDF1 ?  That's simpler than load into W and AND. -iwj
 
        bnz     advance_read_buffer
        bra     advance_read_byte
@@ -334,6 +341,8 @@ advance_pointer
 
 
 advance_read_buffer
+;*** I suggest swapping this and advance_read_byte round, since
+;*** advance_read_byte is a more `inner' bit of the `loop' -iwj
 
 ; move pointer to next buffer 
 
@@ -359,6 +368,7 @@ totrack_overflow
 ; write na=cb=1 and return
 
        bra     one_bit_to_track
+;*** oh, we always introduce a one bit in between ?  *confused* -iwj
        
 
        
@@ -376,9 +386,13 @@ advance_read_byte
        movwf   FSR1H,0                 ; set high byte of IND1 pointer
        movff   INDF1,W,0
        andwf   TOTRACKBIT,0,0          ; mask out bit to be transmitted
+;*** This is the same code as the 2nd stanza in read_from_buffer,
+;*** above.  Surely it should be made common (eg a subroutine) ? -iwj
 
        bz      zero_bit_to_track 
        bra     one_bit_to_track
+;*** zero_bit_to_track and one_bit_to_track end up calling advance_bit -
+;*** is that right ? *confused* -iwj
        
 
 

diff --git a/detpic/nmra-stream.asm b/detpic/nmra-stream.asm
index 00e20057a7a1d6d69522ff255b84a953ac7ed050..97716aa5a46a06cf45247b5f36ed35493bf97a6d 100644 (file)
--- a/detpic/nmra-stream.asm
+++ b/detpic/nmra-stream.asm
@@ -289,6 +289,10 @@ read_from_buffer
        movwf   FSR1H,0                 ; set high byte of IND1 pointer
        movff   INDF1,W,0
        andwf   TOTRACKBIT,0,0          ; mask out bit to be transmitted
+;*** to `mask out' means to clear, eg
+;*** if you were to `mask out bit 0 of 0xff' you'd get 0xfe
+;*** I suggest `test bit to be transmitted' or `check bit ...' or maybe
+;*** `select bit ...'  -iwj
 
        bz      zero_bit_to_track 
        bra     one_bit_to_track
@@ -306,10 +310,12 @@ one_bit_to_track
 
 
 advance_bit
-; rotate tranmitbit to next position 
+; rotate transmitbit to next position 
 
        rrncf   TOTRACKBIT,1,0          ; rotate mask right
-
+;*** surely rrnc (`rotate right not through carry' I assume)
+;*** will leave a copy of the top bit in the N flag ?  Then you
+;*** can use branch if negative.  -iwj
        btfss   TOTRACKBIT,7,0
        call    advance_pointer
 
@@ -327,6 +333,7 @@ advance_pointer
        movwf   FSR1H,0         
        movff   INDF1,W,0
        andlw   0x80
+;*** bit test bit 7 of INDF1 ?  That's simpler than load into W and AND. -iwj
 
        bnz     advance_read_buffer
        bra     advance_read_byte
@@ -334,6 +341,8 @@ advance_pointer
 
 
 advance_read_buffer
+;*** I suggest swapping this and advance_read_byte round, since
+;*** advance_read_byte is a more `inner' bit of the `loop' -iwj
 
 ; move pointer to next buffer 
 
@@ -359,6 +368,7 @@ totrack_overflow
 ; write na=cb=1 and return
 
        bra     one_bit_to_track
+;*** oh, we always introduce a one bit in between ?  *confused* -iwj
        
 
        
@@ -376,9 +386,13 @@ advance_read_byte
        movwf   FSR1H,0                 ; set high byte of IND1 pointer
        movff   INDF1,W,0
        andwf   TOTRACKBIT,0,0          ; mask out bit to be transmitted
+;*** This is the same code as the 2nd stanza in read_from_buffer,
+;*** above.  Surely it should be made common (eg a subroutine) ? -iwj
 
        bz      zero_bit_to_track 
        bra     one_bit_to_track
+;*** zero_bit_to_track and one_bit_to_track end up calling advance_bit -
+;*** is that right ? *confused* -iwj