New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improved shiftIn #63

Closed
nerdralph opened this Issue May 28, 2018 · 23 comments

Comments

Projects
None yet
2 participants
@nerdralph

nerdralph commented May 28, 2018

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

This comment has been minimized.

Owner

MCUdude commented May 28, 2018

Perfect, thanks a lot!

@MCUdude MCUdude closed this in d437110 May 28, 2018

@MCUdude

This comment has been minimized.

Owner

MCUdude commented Jun 18, 2018

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.

@MCUdude MCUdude reopened this Jun 18, 2018

@nerdralph

This comment has been minimized.

nerdralph commented Jun 18, 2018

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

This comment has been minimized.

Owner

MCUdude commented Jun 18, 2018

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

This comment has been minimized.

nerdralph commented Jun 18, 2018

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

This comment has been minimized.

Owner

MCUdude commented Jun 18, 2018

(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

This comment has been minimized.

nerdralph commented Jun 18, 2018

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

This comment has been minimized.

Owner

MCUdude commented Jun 18, 2018

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

This comment has been minimized.

nerdralph commented Jun 18, 2018

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

This comment has been minimized.

Owner

MCUdude commented Jun 18, 2018

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

This comment has been minimized.

nerdralph commented Jun 18, 2018

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

This comment has been minimized.

nerdralph commented Jun 18, 2018

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

This comment has been minimized.

Owner

MCUdude commented Jun 18, 2018

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

@nerdralph

This comment has been minimized.

nerdralph commented Jun 18, 2018

OK. That helps narrow things down a bit.

@nerdralph

This comment has been minimized.

nerdralph commented Jun 19, 2018

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

This comment has been minimized.

Owner

MCUdude commented Jun 19, 2018

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

This comment has been minimized.

nerdralph commented Jun 19, 2018

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

This comment has been minimized.

nerdralph commented Jun 20, 2018

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

This comment has been minimized.

nerdralph commented Jun 21, 2018

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

This comment has been minimized.

nerdralph commented Jun 22, 2018

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

This comment has been minimized.

nerdralph commented Jun 23, 2018

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

This comment has been minimized.

nerdralph commented Jun 23, 2018

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

This comment has been minimized.

Owner

MCUdude commented Jun 23, 2018

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

@MCUdude MCUdude closed this in 9524323 Jun 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment