confused about a break position of "PS2KeyAdvanced::available()"

[NeoNatural]

I feel that the break on line 946 is executed when the condition on line 932 is not satisfied. So this exit is for 'key_ignore' but not for 'buffer full'. To have an exit for 'buffer full' I think we should put the break here(shown as below).

Is it right?

[techpaul]

First please do not use screenshots of code, you may like the syntax highlighting but it makes it more difficult for others to read or comment on sections of the code.. Use the markers of RETTURN key to get start of a new line Three ~ characters Paste section of code (preferably including function start) Line numbers maybe different for some folks After code start another line with Three ~ characters This will highlight the block of code as code to people.

Secondly if this is a style of putting shortest block of two first it does not matter, that is for each establishment's coding standards documents.

If however this is suspicion of a bug, please note your suggestion would mean that the code would not work without other changes more than moving the 'break;' statements. It would work the OPPOSITE way. Please be aware those buffers and functions have been operating correctly for many people over the years without modification, have you found a corner case or situation where it fails?

For those wanting to see one of the functions it is

uint8_t PS2KeyAdvanced::available()
{
int8_t  i, idx;
uint16_t data;

// check output queue
i = _key_head - _key_tail;
if( i < 0 )
  i += _KEY_BUFF_SIZE;
while( i < ( _KEY_BUFF_SIZE - 1 ) ) // process if not full
  if( key_available() )         // not check for more keys to process
    {
    data = translate();         // get next translated key
    if( data == 0 )             // unless in buffer is empty
      break;
    if( ( data & 0xFF ) != PS2_KEY_IGNORE
// Suggestion is to move break to here ...
            && ( data & 0xFF ) > 0 )
      {
      idx = _key_head + 1;         // point to next space
      if( idx >= _KEY_BUFF_SIZE )  // loop to front if necessary
        idx = 0;
      if( idx != _key_tail )
        {
        _key_buffer[ idx ] = data; // save the data to out buffer
        _key_head = idx;
        i++;                      // update count
        }
      }
    else
// move the following line 'break' to above
      break;                    // exit buffer full

Also to show code block highlighting

[NeoNatural]

Thank you very much for check and reply. I'm sorry for my recklessness. I'm new here and I've learnt a lesson. And I failed to convey my idea because of my awkward screenshots. Here comes a clearer explanation.

    if( ( data & 0xFF ) != PS2_KEY_IGNORE
            && ( data & 0xFF ) > 0 )
      {
      idx = _key_head + 1;         // point to next space
      if( idx >= _KEY_BUFF_SIZE )  // loop to front if necessary
        idx = 0;
// I guess the following if is a check whether the _key_buffer is full
      if( idx != _key_tail )
        {
        _key_buffer[ idx ] = data; // save the data to out buffer
        _key_head = idx;
        i++;                      // update count
        }
// So I might hope to put a branch here to exit for buffer full, if a change needs to be made
        else
          break; 
      }
// the following branch is executed when data indicates a key-ignoring case or data shows illegal value
// So this break is necessary but is not for 'buffer full' according to my understanding  
    else
      break;                    // exit buffer full

I'm not challenging the effectiveness of the code. I just wonder about the branches and why the break is annotated with 'exit buffer full'.

[techpaul]

Actually neither the break you suggest adding or the original one are actually needed, nor the check for if full you commented before.

As the outer While loop processes if there is space in the OUTPUT buffer for one output key code at a time, then only when a key sequence is available does it try to translate one sequence if complete,, the checks are extra safety checks that are actually redundant, the break could move up a block or be removed.

So the while loop could becomes with extra comments

while( i < ( _KEY_BUFF_SIZE - 1 ) ) // process if OUTPUT buffer not full
  if( key_available() )         // now check for more input keys to process
    {
    data = translate();         // get next translated key
    if( data == 0 )             // unless input buffer is empty (incomplete sequence received)
      break;
    if( ( data & 0xFF ) != PS2_KEY_IGNORE     // Check not changes in things we ignore
            && ( data & 0xFF ) > 0 )
      {                                        // Valid data to store at this point
      idx = _key_head + 1;         // point to next space
      if( idx >= _KEY_BUFF_SIZE )  // loop to front if necessary
        idx = 0;
//      if( idx != _key_tail )
//        {
      _key_buffer[ idx ] = data; // save the data to out buffer
      _key_head = idx;
      i++;                      // update count
//        }
      }
//    else
//      break;                    // exit buffer full
    }
  else
    break;                      // exit nothing coming in

Which I will consider testing and updating when I get time

[techpaul]

Tested the fixes and released updated version