6502.org Forum  Projects  Code  Documents  Tools  Forum
It is currently Sat Sep 21, 2024 5:49 am

All times are UTC




Post new topic Reply to topic  [ 10 posts ] 
Author Message
 Post subject: Interesting race hazard
PostPosted: Tue Sep 17, 2024 7:11 pm 
Offline

Joined: Mon Jan 19, 2004 12:49 pm
Posts: 844
Location: Potsdam, DE
I think... This from my PS/2 decoder on the Pico.

PS/2 keyboard generates eleven interrupts per keycode, with between three and eight or so codes per key press and release. On the eleventh interrupt, the received keycode is pushed into a circular buffer and a pointer thereto is updated. If too many data are placed in the buffer without being handled, tough...

Running asynchronously and simultaneously, a conversion routine pulls codes from the buffer and massages them to produce ascii results. If this read pointer and the write pointer are different, it is assumed that there is something in the buffer to process; otherwise, a nul value is returned.

Most of the time, this seems to work well; keys are pressed, decoded, and displayed correctly on screen. But every now and then, too often, it can happen that already entered text is repeated - I speculate that is it often happens before the buffer is ever filled, the remainder of the buffer is filled with zeros which don't produce an output, but that the whole buffer is being replayed incorrectly.

Something odd is going on. The effect is as if the read pointer is being advanced past the write pointer (which would give a whole buffer-length of read != write) but I can't see how. The variables are all static and volatile. The interrupt and the reading code are all on the same core. It's very strange...

Further investigation is required.

Neil


Top
 Profile  
Reply with quote  
PostPosted: Thu Sep 19, 2024 11:34 am 
Offline

Joined: Tue Apr 20, 2010 4:02 pm
Posts: 33
Two questions come to mind...

1) can you show us some code?
2) what is supposed to happen in case of buffer overrun?


Top
 Profile  
Reply with quote  
PostPosted: Thu Sep 19, 2024 4:52 pm 
Offline

Joined: Mon Jan 19, 2004 12:49 pm
Posts: 844
Location: Potsdam, DE
I didn't post the code immediately (a) because it's not 6502, though it's going into a peripheral thereto, and (b) because I wanted to play a little first. This code is for the Pi Pico, building on the Arduino platform (of which I am not a fan: apart from its horrible IDE (poor colour choice, too-small fonts in many places etc) I find that the code doesn't quite appear to follow the rules for either C or C++. It leads to some confusing build errors.)

Code:
/* interrupt handler for PS2_CLK
  * here on a falling edge
  */
void irq_kb_callback(uint16_t gpio, uint32_t events)
{
  if(gpio == PS2_CLK)
  {
    /* arrive here on falling edge of kb clock
     *  but wait... has it been too long since our last clock pulse?
     */
    tick = time_us_32();
    if ((tick - last_tick) > TIMEOUT)
    {
      /* too long, too long... */
      rxkb = 0;
      kbclks = 0;
    }
    last_tick = tick;
    rxkb >>= 1;       /* shift existing data right */
    if (gpio_get(PS2_DAT))
    {
      rxkb |= 0x400;  /* set the next bit if high */
    }
    kbclks ++;        /* we've received another bit */
    if (11 == kbclks)
    {
      //sprintf(tb, "[%02x]", (char)(rxkb >> 1));
      //prints (tb);
      kbclks = 0;     /* we have them all, add to the buffer */
      kb_buf[kb_buf_in++] = (uint8_t)(rxkb >> 1); 
     
      if (kb_buf_in == KBBUFFSIZE)
      {
        kb_buf_in = 0;  /* tough, we lose codes */
      }
    }
  }
}


uint8_t irq_expect_key(void)
{
  /* check to see if a complete kb code word has been received in the buffer
   */
//  gpio_set_irq_enabled (PS2_CLK, GPIO_IRQ_EDGE_FALL, false);
  if (kb_buf_in != kb_buf_out)
  {
    /* there's something in the buffer */
    ret = kb_buf[kb_buf_out++];
    if (kb_buf_out == KBBUFFSIZE)
    {
      kb_buf_out = 0;
    }
  }
//  gpio_set_irq_enabled (PS2_CLK, GPIO_IRQ_EDGE_FALL, true);
  return ret;
}


The gpio_set_irq_enabled ought to kill the interrupts during the buffer test but appear to kill it completely. Possibly I'm simply calling too quickly and the actual interrupts simply aren't happening...

The intent is that interrupts move the receive state machine along one bit at a time; when it's got eleven bits (note the timeout to get rid of unfortunate glitches) it stuffs the received datum in the buffer. irq_get_keycode retrieves codes from the buffer, in case we're delayed by other code happening.

Neil


Top
 Profile  
Reply with quote  
PostPosted: Thu Sep 19, 2024 7:21 pm 
Offline
User avatar

Joined: Wed Feb 14, 2018 2:33 pm
Posts: 1467
Location: Scotland
You probably don't want to wrap the head and tail pointers as you are. I think wrapping the head pointer when the buffer is full is what's causing confusion.

In my code to do stuff like this I firstly calculate the new head pointer (current head + 1, wrap if needed), and if it's the same as the tail, I just return - throw the next data byte away without wrapping the pointer, rather than wrapping and overwriting existing un-read data.

Some (AVR) code for an interrupt driven serial input I have looks like this:

Code:
ISR(USART0_RX_vect)
{
  uint8_t c, next ;

  c    = UDR0 ;
  next = RX_BUFFER_MASK & (rxBuffer.head + 1) ;

// Discard characters rather than overflow the buffer

  if (next != rxBuffer.tail)
  {
    rxBuffer.buf [rxBuffer.head] = c ;
    rxBuffer.head = next ;
  }
}

uint8_t serialGetchar (void)
{
  uint8_t c ;

// Busy wait until some data

  while (rxBuffer.head == rxBuffer.tail)
    ;

// ... then get the last character out of the buffer

  c = rxBuffer.buf [rxBuffer.tail] ;
  rxBuffer.tail = RX_BUFFER_MASK & (rxBuffer.tail + 1) ;

  return c ;
}


In this instance the buffer size is a power of 2 so the mask and & to wrap the pointer works.

Code:
#  define       RX_BUFFER_SIZE  128
#  define       RX_BUFFER_MASK  (RX_BUFFER_SIZE-1)


I also note that in the expect routine, you don't explicitly set ret when there is nothing in the buffer. This may also be an issue unless you explicitly set it elsewhere.


-Gordon

_________________
--
Gordon Henderson.
See my Ruby 6502 and 65816 SBC projects here: https://projects.drogon.net/ruby/


Top
 Profile  
Reply with quote  
PostPosted: Thu Sep 19, 2024 8:59 pm 
Offline

Joined: Mon Jan 19, 2004 12:49 pm
Posts: 844
Location: Potsdam, DE
Thanks Gordon. There's a 'ret = K_NULL' in the code that I accidentally erased here when I was deleting some previous commented-out code.

I suspect you're right about the pointer wrapping; I'll have a think about that. Certainly it's my observation that it happens most often at a time when I might expect the wrap to be happening (most keys produce three data points, and the buffer is 128 bytes at the moment.

I'll sleep on it...

Neil


Top
 Profile  
Reply with quote  
PostPosted: Fri Sep 20, 2024 11:02 am 
Offline

Joined: Mon Jan 19, 2004 12:49 pm
Posts: 844
Location: Potsdam, DE
Not as simple as it first appeared... I think there are interacting complexities from the keycode decoding department, too... I'm going to rescope it to include the key decoding in the interrupt and then buffer the actual key. Otherwise I end up with some very confused keystrokes...

Neil


Top
 Profile  
Reply with quote  
PostPosted: Fri Sep 20, 2024 11:37 am 
Offline
User avatar

Joined: Wed Feb 14, 2018 2:33 pm
Posts: 1467
Location: Scotland
barnacle wrote:
Not as simple as it first appeared... I think there are interacting complexities from the keycode decoding department, too... I'm going to rescope it to include the key decoding in the interrupt and then buffer the actual key. Otherwise I end up with some very confused keystrokes...

Neil


There's a good Ben Eater video on all this - might be worth half an hours watch:

https://www.youtube.com/watch?v=7aXbh9VUB3U

Also things like the Arduino PS/2 Keyboard driver has a lot of this too (Which I'm in the process of diving into for another project)

-Gordon

_________________
--
Gordon Henderson.
See my Ruby 6502 and 65816 SBC projects here: https://projects.drogon.net/ruby/


Top
 Profile  
Reply with quote  
PostPosted: Fri Sep 20, 2024 12:33 pm 
Offline

Joined: Mon Jan 19, 2004 12:49 pm
Posts: 844
Location: Potsdam, DE
Seen that long since, Gordon :mrgreen: It's also worth watching one of his a couple later; https://www.youtube.com/watch?v=dL0GO9SeBh0 in which he discusses some code to decode the result. Which is basically the same as mine, only in assembly rather than C... there aren't too many different ways to decode that KB signal.

Interesting that he handles everything in the interrupt, as I proposed doing upthread. My concern here is that the same chip is handling a number of other interrupts, and some of them have to be serviced quickly... meh, suck it and see.

It does look as though its actually the interrupt not being disabled during the comparison of the pointers that's causing the issue; more research needed there, I think.

There are a couple of places where things might fail:
  • random noise on the clock line adding extra bits to the output and thereby shifting it - resolved by implementing a timeout of a couple of bit periods, and discarding the code. This resynchronises for the next code to arrive.
  • missing keycodes, probably caused by the above. This would have to be resolved in the decoder state machine; at the moment, I'm not sure whether it is or not. I think so, since I ignore key releases with the exception of control/shift/alt releases, but I need to check that further.

To be continued...

Neil


Top
 Profile  
Reply with quote  
PostPosted: Sat Sep 21, 2024 2:16 am 
Offline

Joined: Fri Dec 21, 2018 1:05 am
Posts: 1108
Location: Albuquerque NM USA
This discussion is interesting to me because I'm also bit-banging PS2 CLK and DATA for my beam racing 6502. So I'm looking over my code and see whether I have the corner cases covered. My implementation is a little different in that PS2 CLK does not generate interrupt; the interrupt source is the 31.5KHz horizontal sync and back porch part of the interrupt service routine samples PS2 CLK and shifts DATA bits to form a byte of keyboard data which is write to a 256-byte circular buffer aligned to page boundary. The keyboard data is processed during the vertical retrace period whenever the write circular buffer pointer is not equal to the read pointer. The algorithm worked fairly well, but I found it necessary to reset the PS2 state machine when CLK is high for six 31.5KHz interrupts. If not, the occasional glitches on CLK can throw the state machine out of sync.

Bill


Top
 Profile  
Reply with quote  
PostPosted: Sat Sep 21, 2024 5:09 am 
Offline

Joined: Mon Jan 19, 2004 12:49 pm
Posts: 844
Location: Potsdam, DE
Yes, I've already discovered (and resolved) the random glitches - as with yours, a timeout since the last clock pulse. Your sampling would be less sensitive since you're sampling on level while my interrupt triggers on edge, which can occur with a much shorter glitch than your level sample would require.

I think the main requirement for the timeout is the initial synchronisation because the naive approach of just counting clock bits will never synchronise if it starts in the wrong bit.

The second 'edge case' concern is what happens to the decoder if there is a missing keycode for whatever reason. The three main cases are:
  • the initial keycode is missed
  • the release keycode is missed
  • the release identifier is missed
and I need to make sure they don't tie the state machine in a knot.

But at the moment, it looks as if the problem is down to the way the interrupt and the getkey() routines are interracting, so I need to sort that first.

Neil


Top
 Profile  
Reply with quote  
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 10 posts ] 

All times are UTC


Who is online

Users browsing this forum: No registered users and 9 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to: