improved shiftIn

[nerdralph]

It wasn't as bad as shiftOut, but still fell short of ideal. Here's my improved version (faster and smaller compiled size):

  uint8_t value = 0;
  uint8_t i = 8;
  const uint8_t clkpinMask = _BV(clockPin);
  do
  {
    PINB = clkpinMask; // Toggle clock pin
    if (PINB & 1<<dataPin){ 
      if(bitOrder == MSBFIRST) value |= 0x01;
      else value |= 0x80;
    }
    PINB = clkpinMask; // Toggle clock pin
    if(bitOrder == MSBFIRST) 
      value <<= 1;
    else 
      value >>= 1;
  }
  while(--i);

In addition to reviewing the disassembled compiler output, I tested it with a TM1638 LED&KEY module.

[MCUdude]

Perfect, thanks a lot!

[MCUdude]

I'm experiencing some issues with this code.

uint8_t shiftIn(uint8_t dataPin, uint8_t clockPin, uint8_t bitOrder)
{  
  #if defined(SAFEMODE)
    if(clockPin > 5 || datapin > 5) // Return if pin number is too high
      return;
    if(clockPin < 2)
      turnOffPWM(clockPin); // If it's a PWM pin, make sure PWM is off
    if(dataPin < 2)
      turnOffPWM(dataPin);  // If it's a PWM pin, make sure PWM is off
  #endif  

  uint8_t value = 0;
  uint8_t i = 8;
  const uint8_t clkpinMask = _BV(clockPin);
  do
  {
    PINB = clkpinMask; // Toggle clock pin
    if(PINB & _BV(dataPin))
    { 
      if(bitOrder == MSBFIRST) 
        value |= 0x01;
      else 
        value |= 0x80;
    }
    PINB = clkpinMask; // Toggle clock pin
    if(bitOrder == MSBFIRST) 
      value <<= 1;
    else 
      value >>= 1;
  }
  while(--i);

  return value;

I just received my LED&KEY test board today, and I can't properly read the buttons when using the shiftIn function above. I've tested your example code, but it's not able to read any of the buttons.

[nerdralph]

I tested it with a Pro mini and with a t13, so my first guess would be a possible hardware difference. The TM1638 supports a 3x8 keyscan matrix, so the modules can be wired to use one of the 3 different keyscan rows. I checked a couple other peoples code for the 1638, and the only ones I saw that were wired different were the 16-button modules. I'd try modifying startTx to be public instead of private, then copy TM1638NR::readButtons, changing the loop to log/display each of the four bytes read back.

It's also possible there's an integration issue. I tested with my own modified version of MicroCore. To check that I'll try pulling master from git and re-test.

[MCUdude]

I experienced the exact same issues when trying this code as well if that gives you any hints. It works flawless when using this shiftIn function:

uint8_t shiftIn(uint8_t dataPin, uint8_t clockPin, uint8_t bitOrder)
{  
  #if defined(SAFEMODE)
    if(clockPin > 5 || datapin > 5) // Return if pin number is too high
      return;
    if(clockPin < 2)
      turnOffPWM(clockPin); // If it's a PWM pin, make sure PWM is off
    if(dataPin < 2)
      turnOffPWM(dataPin);  // If it's a PWM pin, make sure PWM is off
  #endif  

   uint8_t value = 0;

   for(uint8_t i = 0; i < 8; ++i)
   {
     PORTB |= _BV(clockPin);  // Set clockPin high
     value |= (!!(PINB & _BV(dataPin)) << ((bitOrder == LSBFIRST) ? i : 7 - i)); // clock in to dataPin
     PORTB &= ~_BV(clockPin); // Set clockPin low
   }
   return value;
}

[nerdralph]

Since you're having the same problem with a different library, that further suggests a wiring difference. With a bit more code, I think I could make readButtons() detect which keyscan column the buttons are on...

[MCUdude]

(I edited my last post; you probably didn't see that) I just think it's strange that the old shiftIn implementation worked just fine with your library and the other code example..

[nerdralph]

OK, I didn't understand that it works with the old shiftIn. That changes things. The first thing I'll do is compare my local version of the optimized shiftIn to what you pushed to git, to make sure they are functionally identical.

[MCUdude]

It partially works with the the new shiftIn implementation. This is the response when I click the various buttons. S1 -> No LED S2 -> No LED S3 -> No LED S4 -> No LED S5 -> LED4 S6 -> LED5 S7 -> LED6 S8 -> LED7

[nerdralph]

The code you have in git matches my local copy. I think the new shiftIn might be too fast; the datasheet for the 1638 specifies a max clock speed of 1Mhz, and my shiftIn can go 1.2Mhz with a t13 clocked at 9.6. It'll take a bit of testing, but it shouldn't be too hard to fix...

[MCUdude]

I think the new shiftIn might be too fast; the datasheet for the 1638 specifies a max clock speed of 1Mhz, and my shiftIn can go 1.2Mhz with a t13 clocked at 9.6.

I tried running the T13 at 600kHz but I'm still seeing the same symptoms. Would be awesome if you'd have a closer look at it!

[nerdralph]

That's useful to know. I'm going over the TM1638 datasheet again, and realize it is a bit more nuanced than I initially thought. Although it clocks data in from the MCU on the rising edge like a '595, it clocks data out to the MCU on the falling edge (whereas the 74165 clocks data out to the MCU on the rising edge).

[nerdralph]

One more question. Does it work when you use the new shiftOut and the old shiftIn, or are you using the old version of both shiftOut and shiftIn? readButtons uses shiftOut followed by shiftIn, so I'd like to know if I should be looking at both of them, or just shiftIn.

[MCUdude]

I've only tested using the new implementation of shiftOut. That in combination with old shiftIn works just fine.

[nerdralph]

OK. That helps narrow things down a bit.

[nerdralph]

I did some tests at 17Mhz (OSCCAL=0xff), and with the old shiftIn, I read zeros for S5-S8. At ~2Mhz, it's fine. So simply going back to the old shiftIn isn't a complete solution. I don't completely trust the TM1638 datasheet, so I'm going to hook up my scope to figure out the exact timing. Then I'll know if I actually have to slow down shiftIn, or just add a delay between sending the readbuttons command and clocking in the data.

[MCUdude]

Good! So it's probably nothing wrong with the shiftIn function itself, it's just a coincidence that the TM1638 doesn't seem to like it. Looking forward to hear from you if/when you find a solution or workaround!

[nerdralph]

Yesterday when I was looking at the datasheet, I started thinking that shiftIn/shiftOut aren't the ideal functions to send/receive data. The TM1638 communication is more like I2C than SPI, as the lines have pullups, and so the clock is sort of inverted compared to SPI. Once I put the scope on the CLOCK & DATA, I could see droop on the CLOCK suggesting TM1638 tries to hold the CLOCK low, like an I2C slave does for clock stretching. I'm re-writing my TM1638 library so it works properly with an open-drain bus, which means it won't use the standard shiftIn and shiftOut functions.

[nerdralph]

I'm almost finished testing the new TM1638 library that uses open-drain communications. So far it's working on the t13, and I'll test it on a m328 before I push it to git. I also think I found a bug in the shiftIn code (value should be shifted at the top of the loop, not the bottom). Since my new TM1638 library no longer uses shiftIn, I'll dig out a 74165 to test the fixed shiftIn.

[nerdralph]

I just pushed the TM1638NR library re-write: https://github.com/nerdralph/nerdralph/tree/master/TM1638NR

Later today I'm hoping to get the shiftIn testing done with a 74HC165.

[nerdralph]

Here's the tested version of shiftIn:

  uint8_t value = 0;
  uint8_t i = 8;
  const uint8_t clkpinMask = _BV(clockPin);
  do
  {
    PINB = clkpinMask; // Toggle clock pin
    if(bitOrder == MSBFIRST) 
      value <<= 1;
    else 
      value >>= 1;
    if (PINB & 1<<dataPin){ 
      if(bitOrder == MSBFIRST) value |= 0x01;
      else value |= 0x80;
    }
    PINB = clkpinMask; // Toggle clock pin
  }
  while(--i);

If you try it with a 74165, you might notice the first input (H) is skipped, but that's because it is clocked differently than a CD4012, which the Arduino code is written for: https://www.arduino.cc/en/Tutorial/ShiftIn

[nerdralph]

Here's a (slightly slower) version of shiftIn that is easier to use with a 74165, and more closely mirrors the functionality of the official shiftIn. The official shiftIn leaves the clock low, while my initial version left the clock in the state that it started; i.e. if the clock was high, it ends high. While I think that's a better way to do it since it makes it simple to handled an inverted clock, it could break code that relies on the way the official shiftIn works.

  do
  {
    digitalWrite(clockPin, HIGH);
    if(bitOrder == MSBFIRST) 
      value <<= 1;
    else 
      value >>= 1;
    if (digitalRead(dataPin)){ 
      if(bitOrder == MSBFIRST) value |= 0x01;
      else value |= 0x80;
    }
    digitalWrite(clockPin, LOW);
  }
  while(--i);

p.s. it is still smaller (18 fewer bytes compiled) and faster than the old MicroCore shiftIn.

[nerdralph]

I just finished a short blog post that explains how to use shiftIn wiith a 74165. http://nerdralph.blogspot.com/2018/06/using-shiftin-with-74165-shift-register.html

[MCUdude]

Awesome, thanks allow for digging down into all this! Really appreciate it 🥇