[FIXED] Possible error in encoder priority system

v2 Custom framework Bug report
#1

I’ve been reading the controller code in hopes of addressing a feedback performance issue I’ve been having, and I noticed something possibly performance-related in EncoderInputs::Read which I’m ~80% sure is a bug.

I think it’s probably not causing my issue but it might be worth looking at. Forgive me if this is the intended behavior and I’m just missing something!

So encoders.ino lines 237-265 handle the “priority system.” I think the idea is to avoid checking all encoder modules while one or two are being actively used. 237-240 handle the timeout and 241-265 pick which modules to read over SPI using digitalRead().

Here’s the logic in v0.13 (where priorityCount can only be 0, 1 or 2):

  // The priority system adds to a priority list up to 2 modules to read if they are being used.
  // If the list is empty, the first two modules that are detected to have a change are added to the list
 // If the list is not empty, the only modules that are read via SPI are the ones that are on the priority list
  if(priorityCount >= 1) {
      // read the first priority module, if there is one
  }
  if(priorityCount == 2){
      // read the second priority module, if there is one
  }else{  
      // READ ALL MODULE'S STATE
      for (int n = 0; n < nModules; n++){
           // read the n-th module's state
      }
   }

Isn’t that reaching the third code block and therefore reading all modules when priorityCount==1, including a second read of whatever the first priority module was? I suspect it was written this way to avoid repeating the first module read in the second module’s case, but to match the comment it seems like it should be:

  if(priorityCount >= 1) {
      // read pri #1
  }
  if(priorityCount == 2){
      // read pri #2
  }else if (priorityCount == 0){  
     // read all
   }

Or you just flip the order and do:

  if(priorityCount == 2) {
      // read #2
  }
  if(priorityCount >= 1){
      // read #1
  }else{  
      // read all
   }

Please forgive me if I’m misunderstanding the goal (or the code) here! Just thought I’d mention it.

#2

Hi @gc3!

Yeah I think you’re right, actually i think sthg like this is what I intended, which would also work:

// The priority system adds to a priority list up to 2 modules to read if they are being used.
// If the list is empty, the first two modules that are detected to have a change are added to the list
// If the list is not empty, the only modules that are read via SPI are the ones that are on the priority list
if(priorityCount >= 1) {
  // read the first priority module, if there is one
  if(priorityCount == 2){
    // read the second priority module, if there is one
  }
}else{  
  // READ ALL MODULE'S STATE
  for (int n = 0; n < nModules; n++){
       // read the n-th module's state
  }
}

I also don’t think this is causing the issue you’re having with the feedback, and I will answer on the other post my thoughts!

1 Like
#3

@francoytx, thanks again!

I tested the 3rd version and it looks like it stops anything from getting added to the list since the only way priorityCount can go above 0 is if a read happens, which can only happen if priorityCount >= 1. Went with the 1st version (using else if) and it actually does seem to make some difference, although it’s hard to tell (I don’t have an objective way of measuring the issue yet).

It creates another issue, though–I don’t think the priorityCount ever gets to 2, since once it hits 1 it’s only reading from that module until timeout, so a second module can’t get added. If I turn two knobs on the same module at the same time, they both read, but if I turn two knobs on two different modules at the same time, one “wins” and the other doesn’t update and is locked out until the priority timer releases.

I think I have a fix (basically interleaving the read and compare steps) but I only have a few mins now and want to poke a little more at the other problem (and see if there is a relationship or if it’s just wishful thinking!) Will come back to this later unless you’ve poked at it first :slight_smile:

#4

Mmmm i would think a read should also happen if priorityCount == 0 because of the else statement

if(priorityCount >= 1) {
    // Read with priority one or two modules
}else{  
    // READ ALL MODULE'S STATE
    for (int n = 0; n < nModules; n++){
       // read the n-th module's state
    } 
}

But yeah, this version won’t for sure add a 2nd priority module

The 1st version looks alright though! Also have to find a workaround to add a 2nd priority module.

The idea behind 2 prioritized modules is that you cannot turn more than 2 encoders at the same time, so more wouldn’t be necessary.

This code was done at least a year ago, so I’m having trouble remembering all the reasons for it being like this, but I suspect that the original version intended to keep polling other modules to know if there should be a second module added to priority, in which case you are right to say there is a double read of the first prioritized module.
We could also exclude this index from the for cycle.

#5

Makes sense & sounds good! I just meant that the version with enclosing blocks (below) doesn’t work because it can’t ever read anything when priorityCount==0. I think the one that puts the 2 test first should behave identically to the 1st ver.

if(priorityCount >= 1) {
       // read the first priority module, if there is one
     if(priorityCount == 2){
      // read the second priority module, if there is one
    }
  }else{  
  // READ ALL MODULE'S STATE
  for (int n = 0; n < nModules; n++){
       // read the n-th module's state
  }
}

So with the intended behavior the maximum amount of SPI reading is actually happening when no encoders are being turned. The performance theory (if I understand correctly) is that more is going on in other areas when an encoder is actually being updated, so you want to reduce the SPI reads when that’s happening to offset the load?

That sounds solid w/the two-module limit working. I guess you could do a sweep with your hand, but nobody’s hand is going to span more than two modules (so that would work fine–all encoders on both modules would be updated), and I think two sweeps with two hands (or one arm on a large-format controller) spanning three or four modules total would be pretty rare!

#6

Also it makes the read of this encoder modules faster (the loop will be shorter).

This makes the encoder reads more reliable and less states of the encoder turning are missed.

I will give this a go tomorrow and report back.

1 Like