Nice to see you're using these macros I wrote months ago.  At the time, David seemed interested to include them in Arduino.  At least that's what he said on the developer mail list.  I would not have done all that work had I known it would only sit in the issue tracker, rather than be included in 0018.  (likewise, many other optimizations I've explored won't been ported to Arduino and written up nicely, because if issue #140 can't be included, certainly other more difficult optimizations for non-const cases won't be).

Still, a library implementation is better than nothing.

Could I convince you to surround each definition with a check to avoid redefining if it already exists?  For example:

#if !defined(digitalPinToPortReg)
#define digitalPinToPortReg(P) \
(((P) >= 0 && (P) <= 7) ? &PORTD : (((P) >= 8 && (P) <= 13) ? &PORTB : &PORTC))
#endif

I realize this adds a lot of extra #if and #endif lines.  However, if these definitions ever become included in the Arduino core (where they rightly belong), your code will continue to work.

On 3rd party boards, this will allow those boards to define these macros for their different pin assignments, and your code will automatically use them.


Also, on Arduino Mega, issue #146 will apply to the pins which use registers beyond the range usable with the CBI and SBI instructions.  Then again, issue #146 applies to ALL usage of the normal digitalWrite() and pinMode() functions, regardless of register addresses.

If you care about issue #146, you could add a check if the pointer (cast to an integer) is greater than 32, and if so, surround the bitWrite with code to save interrupt context, disable interrupts, and then restore.  Because this is within the check for __builtin_constant_p(P), the compiler will only include that code for the appropriate pins.

So far, nobody seems to care much about issue #146.  Someday I'll get around to writing some test cases to demonstrate the problem.  Trust me, it is real.  The "nobody has ever complained in 5 years" is only because recently have widely used libraries and functions called digitalWrite() from interrupts, and because the problems are so very mysterious and difficult to debug.  Such is the way of race conditions.

Still, nice work on library-izing this.  Hope it gets some use.

Very nice.  The code's looking very good.  I'll try to come up with a subclass example on send().

Here's a list of minor little things I noticed looking at the latest code.  Nothing's critical, just little things.


_busyPin seems to always be _data_pins[3].  Eliminating it might reduce code size, and save an extra byte of per-instance RAM.

rwSave save in init() appears to be unused now.

Inside init(), should en2 be checked for 255 instead of 0 to see if it's unused?

Inside init2(), it would be advisable to call delayMicroseconds with only 15000 and do the loop 9 times instead of 3.  Even though delayMicroseconds takes a 16 bit input, it doesn't actually work properly beyond 16383.  In fact, limiting your call to 8191 us might be a good idea, in anticipation of a 32 MHz AVR (eg, the xmega chips).

Can _displayfunction become only a local variable inside begin2(), possibly saving code side and one more byte of per-instance allocated RAM?

Yes, I would agree, this version is probably about as fast as it's ever going to get, at least without ugly tricks like hard coded pins or assembly language.

You've already incorporated so much feedback from me and others, and I can understand wanting to do one last round of final testing and wrap everything up.  But if I could talk you into reconsidering the userbusy API again, I just hope there's a simpler way.

Perhaps instead of userbusy, the same goals could be accomplished by making send() public?  Then an example could be crafted which creates a subclass where send() overridden?  That would probably be even faster than userbusy, since the 8 digitalWrite() and enable pulse could use digitalWriteFast, as well as reading the busy bit and switching pin modes.

Regarding not using 8 bit mode, perhaps it could be perceived as "sneaky", but if it's well documented that the 4 extra wires aren't actually used, that seems perfectly fair.  I believe pretty much anybody would appreciate the code being much smaller and simpler.  If they care about speed, I think they'll really appreciate all your hard work that's gone into benchmarking that proves using only 7 pins is actually fastest.

It's really great work you've done here.  A lot of users are going to benefit once this gets merged into an official Arduino release!

That bit of trickiness does not get used in write8bits. I suppose that implies that the size of the routine has gotten beyond what the optimizer looks at. Maybe it would be vice versa if I'd reversed which method was in 4bits and which one was in 8bits.

Most likely it's the call to pulseEnable that prevents the compiler from applying the same optimizations.  If you try moving it outside write8bits, there's a pretty good chance you'll see similar optimization as write4bits.

If the userFunc check were dropped, inlining checkLcdBusyFlag or just folding that code into send() might speed things up too?  It writes to rs_pin, so it'd go before the digitialWrite to _rs_pin at the beginning of send... fitting together with the idea of moving the rs_pin wiring out of write4bits and eliminating passing it as a constant all the way through the call hierarchy.  That would get rid of a lot of redundant writes to rs_pin as well, since there be only 1 in each copy of send (send_data, send_cmd) right after the busy check/wait.  If the checkLcdBusyFlag code is folded into send, the case where _rw_pin is used, in send_cmd it won't be necessary to re-write rs_pin because the busy check leaves it low.  Unfortunately, the speed critical data case will still need it written to high after the busy check, but this should still be a net reduction in digitalWrite calls, plus savings in function calls and parameter passing.

I believe might be more opportunities for speed improvement.

I downloaded the "LiquidCrystalBulletproof.zip", which appears to be the code before unrolling those loops.  Any chance you could upload the latest?  Could you also upload the benchmark program, or maybe even add it into the examples directory?

Here's a few more ideas...

Why are 3 digitalWrite calls necessary in pulseEnable?  Would it make more sense to write en LOW inside init, and then require only 2 calls to digitalWrite?  Especially on Arduino where digitalWrite is so slow, this could really help.

Perhaps writing to the rs_pin could be moved out of write4bits and write8bits, one level up in call hierarchy?  In 4 bit mode, that will avoid needlessly writing it twice, plus the extra overhead of passing it into another function, twice.  It looks like send and init are the only places that call write4bits, and multiple calls all pass the same value.  Who knows, this might even reduce code size?

Maybe pulseEnable could be folded into write4bits?  However, I'd expect the improvement may be minor, as the compiler is already doing optimizations there.

It might even make sense to create 2 send functions, like send_data() and send_cmd().  Every call to send() passes either HIGH or LOW constants.  Together with the idea of moving the rs_pin write up into send(), it would eliminate lots of parameter passing that is only constants.

In the the 1st line of write(), is _setCursFlag more likely to be zero?  If so, testing it first would be a win.  Then again, I'm not easily seeing how _scroll_count changes, but if either of these is usually zero, you want to test the one more likely to be zero first, since the compiler skips the 2nd test if the first indicates the condition won't call setCursor.

Also perhaps worth trying might be calling setCursor() in the only 2 places where _setCursFlag is set to 1.  Then again, if the user is writing strings that they know fit, which seems likely, this could be slower?

Ok, that's all the ideas I have right now, but I'll probably think of more.....

digitalWrite takes 0 for low and any non-zero value for high, so instead of "( v >>= 1) & 01", you could use "v & 2", "v & 4" and "v & 8".

Or if the compiler doesn't generate efficient code for that, this might coax it into using the single LD with post inc instruction for the first arg, and a single instruction for the second, so each line should compile to only 2 instructions plus the call.

Code:

   const uint8_t *pinptr = _data_pins;
    pinMode(*pinptr++,INPUT);
    pinMode(*pinptr++,INPUT);
    pinMode(*pinptr++,INPUT);
    pinMode(*pinptr++,INPUT);
    if (_data_pinNumber > 4) {
        pinMode(*pinptr++,INPUT);
        pinMode(*pinptr++,INPUT);
        pinMode(*pinptr++,INPUT);
        pinMode(*pinptr++,INPUT);
    }

It really should allocate "pinptr" to Y (r28 & r29).  If it doesn't, there an asm syntax that can be added to force allocating to Y, which isn't clobbered by the call to pinMode.

Just a quick idea... would rwBusy() be faster if you unrolled the two loops for the pinMode.  For example, instead of this:

Code:

   for (uint8_t i = 0; i < _data_pinNumber; i++) pinMode(_data_pins[i],INPUT);

maybe something like this could be faster?

Code:

   pinMode(_data_pins[0],INPUT);
    pinMode(_data_pins[1],INPUT);
    pinMode(_data_pins[2],INPUT);
    pinMode(_data_pins[3],INPUT);
    if (_data_pinNumber > 4) {
        pinMode(_data_pins[4],INPUT);
        pinMode(_data_pins[5],INPUT);
        pinMode(_data_pins[6],INPUT);
        pinMode(_data_pins[7],INPUT);
    }

If the _data_pins were static, the compiler would likely just insert direct memory addresses.  But maybe it'll be smart and use the Y pointer with displacement instruction?  Maybe not?

[3 servo motors]

I would like to propose an alternate implementation.

This code supports 3 servo motors on chips with 3 timer1 PWM channels (such as the Arduino Mega).

It also defines names SERVO_PIN_A and SERVO_PIN_B which can be used instead of 9 and 10, if you want to write sketches that work on multiple boards.  Of course, it's fully compatible with the old API and you can still use hard-coded pin numbers in your sketch, if you like.

The changes to the original code are based on Zach's approach.  This code supports all '168 and '328 based Arduinos, the Arduino Mega, Sanguino, and both Teensy boards, and it is designed to make adding other boards easy.  The board-specific #defines are in only one location inside servo.h, rather than scattered throughout the actual code.

I'll post this to the developer list too.

Servo.cpp

Code:

#include <avr/interrupt.h>
#include <wiring.h>
#include <Servo.h>

/*
  Servo.h - Hardware Servo Timer Library
  Author: Jim Studt, jim@federated.com
  Copyright (c) 2007 David A. Mellis.  All right reserved.

  This library is free software; you can redistribute it and/or
  modify it under the terms of the GNU Lesser General Public
  License as published by the Free Software Foundation; either
  version 2.1 of the License, or (at your option) any later version.

  This library is distributed in the hope that it will be useful,
  but WITHOUT ANY WARRANTY; without even the implied warranty of
  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  Lesser General Public License for more details.

  You should have received a copy of the GNU Lesser General Public
  License along with this library; if not, write to the Free Software
  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
*/


uint8_t Servo::attachedA = 0;
uint8_t Servo::attachedB = 0;
#ifdef SERVO_PIN_C
uint8_t Servo::attachedC = 0;
#endif

void Servo::seizeTimer1()
{
  uint8_t oldSREG = SREG;

  cli();
  TCCR1A = _BV(WGM11); /* Fast PWM, ICR1 is top */
  TCCR1B = _BV(WGM13) | _BV(WGM12) /* Fast PWM, ICR1 is top */
  | _BV(CS11) /* div 8 clock prescaler */
  ;
  OCR1A = 3000;
  OCR1B = 3000;
  ICR1 = clockCyclesPerMicrosecond()*(20000L/8);  // 20000 uS is a bit fast for the refresh, 20ms, but
                                                  // it keeps us from overflowing ICR1 at 20MHz clocks
                                                  // That "/8" at the end is the prescaler.
#if defined(__AVR_ATmega8__)
  TIMSK &= ~(_BV(TICIE1) | _BV(OCIE1A) | _BV(OCIE1B) | _BV(TOIE1) );
#else
  TIMSK1 &=  ~(_BV(OCIE1A) | _BV(OCIE1B) | _BV(TOIE1) );
#endif

  SREG = oldSREG;  // undo cli()    
}

void Servo::releaseTimer1() {}

#define NO_ANGLE (0xff)

Servo::Servo() : pin(0), angle(NO_ANGLE) {}

uint8_t Servo::attach(int pinArg)
{
  return attach(pinArg, 544, 2400);
}

uint8_t Servo::attach(int pinArg, int min, int max)
{
  #ifdef SERVO_PIN_C
  if (pinArg != SERVO_PIN_A && pinArg != SERVO_PIN_B && pinArg != SERVO_PIN_C) return 0;
  #else
  if (pinArg != SERVO_PIN_A && pinArg != SERVO_PIN_B) return 0;
  #endif

  min16 = min / 16;
  max16 = max / 16;

  pin = pinArg;
  angle = NO_ANGLE;
  digitalWrite(pin, LOW);
  pinMode(pin, OUTPUT);

  #ifdef SERVO_PIN_C
  if (!attachedA && !attachedB && !attachedC) seizeTimer1();
  #else
  if (!attachedA && !attachedB) seizeTimer1();
  #endif

  if (pin == SERVO_PIN_A) {
    attachedA = 1;
    TCCR1A = (TCCR1A & ~_BV(COM1A0)) | _BV(COM1A1);
  }

  if (pin == SERVO_PIN_B) {
    attachedB = 1;
    TCCR1A = (TCCR1A & ~_BV(COM1B0)) | _BV(COM1B1);
  }

  #ifdef SERVO_PIN_C
  if (pin == SERVO_PIN_C) {
    attachedC = 1;
    TCCR1A = (TCCR1A & ~_BV(COM1C0)) | _BV(COM1C1);
  }
  #endif
  return 1;
}

void Servo::detach()
{
  // muck with timer flags
  if (pin == SERVO_PIN_A) {
    attachedA = 0;
    TCCR1A = TCCR1A & ~_BV(COM1A0) & ~_BV(COM1A1);
    pinMode(pin, INPUT);
  }

  if (pin == SERVO_PIN_B) {
    attachedB = 0;
    TCCR1A = TCCR1A & ~_BV(COM1B0) & ~_BV(COM1B1);
    pinMode(pin, INPUT);
  }

  #ifdef SERVO_PIN_C
  if (pin == SERVO_PIN_C) {
    attachedC = 0;
    TCCR1A = TCCR1A & ~_BV(COM1C0) & ~_BV(COM1C1);
    pinMode(pin, INPUT);
  }
  #endif

  #ifdef SERVO_PIN_C
  if (!attachedA && !attachedB && !attachedC) releaseTimer1();
  #else
  if (!attachedA && !attachedB) releaseTimer1();
  #endif
}

void Servo::write(int angleArg)
{
  uint16_t p;

  if (angleArg < 0) angleArg = 0;
  if (angleArg > 180) angleArg = 180;
  angle = angleArg;

  // bleh, have to use longs to prevent overflow, could be tricky if always a 16MHz clock, but not true
  // That 8L on the end is the TCNT1 prescaler, it will need to change if the clock's prescaler changes,
  // but then there will likely be an overflow problem, so it will have to be handled by a human.
  p = (min16*16L*clockCyclesPerMicrosecond() + (max16-min16)*(16L*clockCyclesPerMicrosecond())*angle/180L)/8L;
  if (pin == SERVO_PIN_A) OCR1A = p;
  if (pin == SERVO_PIN_B) OCR1B = p;
  #ifdef SERVO_PIN_C
  if (pin == SERVO_PIN_C) OCR1C = p;
  #endif
}

uint8_t Servo::read()
{
  return angle;
}

uint8_t Servo::attached()
{
  if (pin == SERVO_PIN_A && attachedA) return 1;
  if (pin == SERVO_PIN_B && attachedB) return 1;
  #ifdef SERVO_PIN_C
  if (pin == SERVO_PIN_C && attachedC) return 1;
  #endif
  return 0;
}

Servo.h

Code:

#ifndef Servo_h
#define Servo_h

/*
  Servo.h - Hardware Servo Timer Library
  Author: Jim Studt, jim@federated.com
  Copyright (c) 2007 David A. Mellis.  All right reserved.

  This library is free software; you can redistribute it and/or
  modify it under the terms of the GNU Lesser General Public
  License as published by the Free Software Foundation; either
  version 2.1 of the License, or (at your option) any later version.

  This library is distributed in the hope that it will be useful,
  but WITHOUT ANY WARRANTY; without even the implied warranty of
  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  Lesser General Public License for more details.

  You should have received a copy of the GNU Lesser General Public
  License along with this library; if not, write to the Free Software
  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
*/

#include <inttypes.h>

#if defined(__AVR_ATmega168__) || defined(__AVR_ATmega328P__) // Arduino
  #define SERVO_PIN_A 9
  #define SERVO_PIN_B 10
#elif defined(__AVR_ATmega1280__) // Arduino Mega
  #define SERVO_PIN_A 11
  #define SERVO_PIN_B 12
  #define SERVO_PIN_C 13
#elif defined(__AVR_ATmega644P__) || defined(__AVR_ATmega644__) // Sanguino
  #define SERVO_PIN_A 13
  #define SERVO_PIN_B 12
#elif defined(__AVR_AT90USB162__) // Teensy
  #define SERVO_PIN_A 17
  #define SERVO_PIN_B 18
  #define SERVO_PIN_C 15
#elif defined(__AVR_AT90USB646__) // Teensy++
  #define SERVO_PIN_A 25
  #define SERVO_PIN_B 26
  #define SERVO_PIN_C 27
#else
  #define SERVO_PIN_A 9
  #define SERVO_PIN_B 10
#endif

class Servo
{
  private:
    uint8_t pin;
    uint8_t angle;       // in degrees
    uint8_t min16;       // minimum pulse, 16uS units  (default is 34)
    uint8_t max16;       // maximum pulse, 16uS units, 0-4ms range (default is 150)
    static void seizeTimer1();
    static void releaseTimer1();
    static uint8_t attachedA;
    static uint8_t attachedB;
    #ifdef SERVO_PIN_C
    static uint8_t attachedC;
    #endif
  public:
    Servo();
    uint8_t attach(int);
                             // pulse length for 0 degrees in microseconds, 544uS default
                             // pulse length for 180 degrees in microseconds, 2400uS default
    uint8_t attach(int, int, int);
                             // attach to a pin, sets pinMode, returns 0 on failure, won't
                             // position the servo until a subsequent write() happens
                             // Only works for 9 and 10.
    void detach();
    void write(int);         // specify the angle in degrees, 0 to 180
    uint8_t read();
    uint8_t attached();
};

#endif

I'm curious, are you using the new 2.0 version of OneWire, where I fixed the interrupt safety issues?

Teensyduino has similar code for delayMicroseconds.  I used an inline function and I inlined the var case in asm too.  Your macro approach nicely eliminates a lot of the special handling for each speed.  I do like how you inline non-looping code that doesn't require any registers for under 8 cycles.  Nice!  I might try that idea someday.  I made 2 looped copies, so the short delays only require a single register, but then zero registers is even better.

Have you looked at how the compiler implements the var case multiplication by clockCyclesPerMicrosecond if the variable is a signed char, signed int or signed long?

Each time you want the ADC to measure the voltage on one of the analog input pins, you need to use analogRead() in your sketch to make it do so.  It only does the conversions when you use analogRead(), not automatically.

This is exactly the same as on a regular Arduino board.

Hi, Paul here, creator of the Teensy boards....

It does.  There's also a "Teensy_Firmata" in my installation.  I'm not sure where it came from but I suspect it was installed with the other Teensy support stuff.  From a quick glance, it looks like the Servo stuff and some sort of EEPROM thing is not included in Teensy_Firmata.

Uploading StandardFirmata and Teensy_Firmata seems to have worked; there weren't any errors.

I tried using Processing to communicate with the Teensy+Firmata but I don't know what I'm doing, got a whole bunch of null-pointer errors, and don't have the time to figure out what's wrong.

Recently I've been working with Hans on hardware abstraction in StandardFirmata.  Please contact me via email, paul@pjrc.com

There is an alpha test version that we REALLY could use your feedback on.  I have tested it quite a lot on Teensy boards, and on Arduino Mega and normal Arduino ('328 chip), but I can't try everything, and I'm way too "close" to the code to discover lots of other usage cases.

The plan is to merge this into a future version of Firmata.  As well as Teensy, this will FINALLY bring Firmata to Arduino Mega and it will make adding other boards a lot easier in the future.

But right now, if you're actually using Firmata for anything and you have a Teensy or Arduino Mega or even a Sanguino that you'd like to use, please do contact me by email.


ps: the "Teensy_Firmata" was my earlier port, which works, but it's based on an older version of Firmata (as are some of the other examples shipped with Firmata).  In the long run, hopefully with some help from people to test, StandardFirmata will support all boards and adding new ones will be much easier in the future.

Just a quick followup...

After much investigation, it turned out Winbatch has a bug where it erroneously sends the break signal when opening any serial port.

Teensy uses the break signal as its command to reboot, so every time Winbatch tried to open the port, Teensy would reboot.  Because it's native on-chip USB instead of a USB-serial converter, the port would disappear right as Winbatch was opening it.  The Arduino serial monitor and other terminal emulators would work fine.

On a normal Arduino, DTR from the USB-serial chip reboots the processor.  That obviously isn't an option for Teensy since rebooting means also rebooting the USB stack, so the break signal was used instead.

It also turns out the Mac OS-X USB serial driver has a bug where it can't send the break signal.  You can call tcsendbreak() and OS-X returns a success status code, but nothing is actually transmitted.  So to make OS-X work, reboot upon setting the baud rate to 134 bits/sec was implemented.

In the latest "experimental" 0.9 Teensyduino code, I have switched Windows and Linux support to use 134 baud setting instead of the break signal, and the reset-on-break behavior has been removed.

This should allow Teensy to work with Winbatch, or any other programs that unexpectedly sends the break signal.  Just don't set the baud rate to 134.  On Teensy, the data transfer is always native 12 Mbit/sec with native flow control, no matter what the baud rate setting in software.

I have been working to bring Firmata to other boards, including Arduino Mega and Sanguino.

Please please please give this latest code a try (especially on a Sanguino, since I don't have one)

http://firmata.org/wiki/Proposals#User_Friendly_GUI_Queries

Currently this process is more-or-less stalled for lack of user testing needed for Hans to have the confidence to release.


ps: please be aware that many "legacy" Firmata implementation (eg, AS3Glue) may only support the pins present on normal Arduino boards, even though Firmata itself is capable of much more.