MiniCore icon indicating copy to clipboard operation
MiniCore copied to clipboard

use shiftIn from MicroCore

Open nerdralph opened this issue 6 years ago • 8 comments

Since it is smaller and faster. With a few changes, shiftOut could be used as well. For MicroCore I used direct PORTB access in the optimized shiftOut because I didn't want to take the time to figure out the digitalPinToPort function or whatever it is called.

nerdralph avatar Jun 23 '18 14:06 nerdralph

On paper it's a good idea, but the key is compatibility. I want MiniCore to be as close to the original source code in terms of performance, even if the original source code is unnecessary slow.

MicroCore have to be as efficient as possible to even fit a small program.

However if the shiftIn and shiftOut functions were rewritten in the original source code I'd definitly update it in the MiniCore source code as well.

MCUdude avatar Jun 23 '18 17:06 MCUdude

I think you are mistaken. For example, Wiring's digitalWrite() is much faster than Arduino's, without any apparent compatibility problems. Even with the official Arduino, the speed of digital IO can vary significantly depending on the hardware (8/16Mhz), and the version of avr-gcc used by the core. But it's your project, so you are free to keep it as slow and bloated as the original Arduino core.

nerdralph avatar Jun 23 '18 17:06 nerdralph

You're probably right. I'll put it on my todo-list. I want to do some testing on it as well. I think I'll have to use digitalPinToPort in order to make it fast enough

MCUdude avatar Jun 23 '18 17:06 MCUdude

I'd keep the digitalWrite calls in my latest version of shiftIn, and use the digitalWrite/pinWrite code from Wiring: https://github.com/WiringProject/Wiring/blob/master/framework/cores/AVR8Bit/WDigital.h

Like MicroCore, Wiring's digitalWrite compiles to a single sbi or cbi instruction when it is used with constant parameters.

In case you didn't know, Arduino forked from Wiring, but didn't keep up with some of the improvements that were made after the fork. Here's the long version of the story: https://arduinohistory.github.io/

nerdralph avatar Jun 23 '18 17:06 nerdralph

So with the implementation if shiftIn with MicroCore it doesn't really matter if I use direct port manipulation or digitalRead/digitalWrite, since it all compiles down to a single ski instruction?

MCUdude avatar Jun 23 '18 17:06 MCUdude

Yes. Here's the asm for shiftIn when compiled with MicroCore:

  46:   88 e0           ldi     r24, 0x08       ; 8
  48:   c0 e0           ldi     r28, 0x00       ; 0
  4a:   c0 9a           sbi     0x18, 0 ; 24
  4c:   cc 0f           add     r28, r28
  4e:   b4 99           sbic    0x16, 4 ; 22
  50:   c1 60           ori     r28, 0x01       ; 1
  52:   c0 98           cbi     0x18, 0 ; 24
  54:   81 50           subi    r24, 0x01       ; 1
  56:   c9 f7           brne    .-14            ; 0x4a <__SREG__+0xb>

The initial version that I wrote using PINB is slightly faster because 'PINB = data' compiles to an out instruction which takes 1 cycle, while sbi & cbi take two. With Arduino's digitalWrite (at least the last time I looked at it), the code compiles to over a dozen instructions.

Here's one (of many) examples of people that looked into the performance issue: https://www.codeproject.com/Articles/732646/Fast-digital-I-O-for-Arduino

nerdralph avatar Jun 23 '18 17:06 nerdralph

Thanks for the tip about using Wiring's code!

MCUdude avatar Jun 23 '18 18:06 MCUdude

Here's a version of shiftOut I just whipped up that would work for both MiniCore and MicroCore. It's about half the speed of the optimized shiftOut I wrote for MicroCore, but on the plus side it compiles to one less instruction on MicroCore.

void shiftO(byte dataPin, byte clockPin, byte bitOrder, byte value){
  byte i = 8;
  do {
    if ( bitOrder == MSBFIRST){
      if(value & 0x80)
        digitalWrite(dataPin, HIGH);
      else
        digitalWrite(dataPin, LOW);
      value <<= 1;
    }
    else{
      if(value & 0x01)
        digitalWrite(dataPin, HIGH);
      else
        digitalWrite(dataPin, LOW);
      value >>= 1;
    }
    digitalWrite(clockPin, HIGH);
    digitalWrite(clockPin, LOW);
  } while(--i);
}

nerdralph avatar Jun 23 '18 18:06 nerdralph