Teensyduino 1.20 Released

[Paul]

Teensyduino 1.20 has been officially released.

http://www.pjrc.com/teensy/td_download.html

 

[duff]

just a reminder, can detachInterruptVector be added to the next Teensyduino version?

 

[jitter]

Just installed the 1.20 version.

Should the USB host shield library be fetched externally? I haven't seen it being included in the installer.

 

[Nantonos]

Yes, and you should put it inside your sketches/libraries folder.

 

[xxxajk]

@jitter:
Good to see out UHS library is in demand.

Just a sneak-leak here, the next version of it is currently in-progress, and will be totally amazing.
You will be even able to support your very own devices with either a special additional driver library provided by yourself, or directly within a sketch.
Reduced code footprint.
Ability to use different SEI (Session Engine Implementation) -- e.g. the teensys own USB port. 1.20 should have the ability now if the code I requested to be pulled in was indeed pulled in, but I have been so busy writing the new UHS code I haven't had the opportunity to check.
If this is not the case, we can just do this next time, and perhaps Paul will include this new version.
However, I'm in no real hurry at this point to verify that is the situation anyway, since the new code has not even been parsed in any compiler yet.

 

[xxxajk]

Paul:

Only one warning in 1.20:

/opt/arduino-1.0.6/libraries/Wire/Wire.cpp: In member function 'uint8_t TwoWire::requestFrom(uint8_t, uint8_t, uint8_t)':
/opt/arduino-1.0.6/libraries/Wire/Wire.cpp:349:10: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]


minor IMHO. Something for the next version.

 

[PaulStoffregen]

If this is not the case, we can just do this next time, and perhaps Paul will include this new version.

Yes, I'd love to include an updated UHS lib with the Teensyduino 1.21 installer. I've been averaging a new version every 3-4 months, so the time frame would probably be in January.

Long term, I'd really also like to help with integrating Teensy 3.1's USB host mode with the library, especially if you're moving to a new design that allows a nice hardware abstraction. Realistically, to develop code by USB, some sort of special USB mux will likely be needed. Maybe it's time to think about what a first gen USB mux board might look like?

 

[xxxajk]

Yes, I'd love to include an updated UHS lib with the Teensyduino 1.21 installer. I've been averaging a new version every 3-4 months, so the time frame would probably be in January.

That's fine. No drivers have been ported to the newer version yet, except hub, and that isn't even tested yet.
I just now (as of this posting)got enumeration to work properly for the MAX chip.

Long term, I'd really also like to help with integrating Teensy 3.1's USB host mode with the library, especially if you're moving to a new design that allows a nice hardware abstraction. Realistically, to develop code by USB, some sort of special USB mux will likely be needed. Maybe it's time to think about what a first gen USB mux board might look like?

No 'mux' board is needed. The Freescale OTG features can be supported on-the-fly.
It is pretty easy to rework the teensy 3.x to support the vbus switch right on the board too.
I also already have host cables for micro USB.
One could simply upload new code via a thumb drive or SDcard, or heck, SPI or even I2C instead via another uC. No real need to limit to just the teensy 3 loader code since we can program right in-place from code copied to RAM.

 

[xxxajk]

One more bug.... won't affect anyone but me, because the code is not released into the public, and I'm holding off until it actually compiles without error.
Basically you forgot an #ifdef statement....
I'll post a pull request on github (when I get to it).
I Also believe that the whole USB handling needs to be separated into an optional library later on so we can support different modes of OTG. It is more-or-less mandatory in order to be able to get at the internals.
Here's the compile error message, it is the only part that fails (did a make -k to examine all compile paths):
/opt/Arduino/hardware/teensy/cores/teensy3/yield.cpp: In function 'void yield()':
/opt/Arduino/hardware/teensy/cores/teensy3/yield.cpp:43:6: error: 'Serial' was not declared in this scope
/opt/Arduino/hardware/teensy/cores/teensy3/yield.cpp:43:38: error: 'serialEvent' was not declared in this scope

OOPS! "USB_SERIAL" isn't defined, and therefore there is no "Serial" class ;-)

Here's a quick 'patch' for this for anyone who wants to do a quick test to verify if it affects them. It shouldn't affect any normal sketch via the IDE.

File /opt/Arduino/hardware/teensy/cores/teensy3/yield.cpp

/* Teensyduino Core Library
 * http://www.pjrc.com/teensy/
 * Copyright (c) 2014 PJRC.COM, LLC.
 *
 * Permission is hereby granted, free of charge, to any person obtaining
 * a copy of this software and associated documentation files (the
 * "Software"), to deal in the Software without restriction, including
 * without limitation the rights to use, copy, modify, merge, publish,
 * distribute, sublicense, and/or sell copies of the Software, and to
 * permit persons to whom the Software is furnished to do so, subject to
 * the following conditions:
 *
 * 1. The above copyright notice and this permission notice shall be
 * included in all copies or substantial portions of the Software.
 *
 * 2. If the Software is incorporated into a build system that allows
 * selection among a list of target devices, then similar target
 * devices manufactured by PJRC.COM must be included in the list of
 * target devices and selectable in the same manner.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
 * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
 * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
 * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
 * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
 * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
 * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 * SOFTWARE.
 */

#include "core_pins.h"
#include "HardwareSerial.h"
#include "usb_serial.h"
#include "usb_seremu.h"

void yield(void) __attribute__ ((weak));
void yield(void)
{
        static uint8_t running=0;

        if (running) return; // TODO: does this need to be atomic?
        running = 1;
#ifdef USB_SERIAL
        if (Serial.available()) serialEvent();
#endif
        if (Serial1.available()) serialEvent1();
        if (Serial2.available()) serialEvent2();
        if (Serial3.available()) serialEvent3();
        running = 0;
};

 

[xxxajk]

I made a better patch and a pull request on github.
https://github.com/PaulStoffregen/cores/pull/37

 

[xxxajk]

Hey Paul, I found a pretty nasty bug in the SPI library. It does not save the interrupt state atomically.

Original method:

        inline static void beginTransaction(SPISettings settings) {
                if (interruptMode > 0) {
                        #ifdef SPI_AVR_EIMSK
                        if (interruptMode == 1) {
                                interruptSave = SPI_AVR_EIMSK;
                                SPI_AVR_EIMSK &= ~interruptMask;
                        } else
                        #endif
                        {
                                interruptSave = SREG;
                                cli();
                        }
                }
                #ifdef SPI_TRANSACTION_MISMATCH_LED
                if (inTransactionFlag) {
                        pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT);
                        digitalWrite(SPI_TRANSACTION_MISMATCH_LED, HIGH);
                }
                inTransactionFlag = 1;
                #endif
                SPCR = settings.spcr;
                SPSR = settings.spsr;
        }

Here is the better version...
It saves SREG to a private variable *first* then, the ***SHARED*** interruptSave variable AFTER the cli(), and thus is atomic.
The original above will race if you try to do beginTransaction from outside the isr, from a sketch, for example.

        inline static void beginTransaction(SPISettings settings) {
                if (interruptMode > 0) {
                        #ifdef SPI_AVR_EIMSK
                        if (interruptMode == 1) {
                                interruptSave = SPI_AVR_EIMSK;
                                SPI_AVR_EIMSK &= ~interruptMask;
                        } else
                        #endif
                        {
                                uint8_t x = SREG;
                                cli();
                                interruptSave = x;
                        }
                }
                #ifdef SPI_TRANSACTION_MISMATCH_LED
                if (inTransactionFlag) {
                        pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT);
                        digitalWrite(SPI_TRANSACTION_MISMATCH_LED, HIGH);
                }
                inTransactionFlag = 1;
                #endif
                SPCR = settings.spcr;
                SPSR = settings.spsr;
        }

Hope you update this soon.

 

[PaulStoffregen]

Do you think the interruptMode == 1 case also needs to be done with all interrupts disabled?

 

[PaulStoffregen]

Yes, I believe you're right.

If the interrupt occurs exactly between the write to interruptSave and cli(), then the interrupt will overwrite interruptSave and when control returns to the main program, it will execute the cli(), do a complete transaction, and then erroneously restore SREG with the value the ISR's transaction saved into interruptSave.

I'm pretty impressed you found this! Thanks.

Looking at this a bit more, it seems the same might be able to happen in the EIMSK code. Maybe? If the interrupt occurs after interruptSave is written, but before EIMSK is written, when the non-interrupt transaction completes last, it could end up restoring EIMSK with the ISR's saved copy.

Maybe something like this is needed? Any thoughts?

                        if (interruptMode == 1) {
                                uint8_t tmp = SREG;
                                cli();
                                interruptSave = SPI_AVR_EIMSK;
                                SPI_AVR_EIMSK &= ~interruptMask;
                                SREG = tmp;
                        } else

 

[xxxajk]

Yes, I believe you're right.

If the interrupt occurs exactly between the write to interruptSave and cli(), then the interrupt will overwrite interruptSave and when control returns to the main program, it will execute the cli(), do a complete transaction, and then erroneously restore SREG with the value the ISR's transaction saved into interruptSave.

I'm pretty impressed you found this! Thanks.

You are quite welcome
Why are you impressed that I found it anyway?
I wrote my code, was experiencing a race in my code, and odd serial behavior. I instantly suspected it was the transaction locking.
Basically, the interrupt pin was acting very strange and had huge LOW pulses that should never be happening.
One look and it was pretty obvious what was going on. If you ever looked at my xmem2 library, you'd know I am very familiar with ISRs and parallel tasking, even with using multiple CPU...
Now....
I think this snag merits a new release. It is a major boo-boo.
How will the average user get the fix?
Are you going to do a quick-fix release? And if you do, can you pull in my git pull request too, which fixes my one last barrier from releasing some really awesome teensy 3.x code/libraries?

Looking at this a bit more, it seems the same might be able to happen in the EIMSK code. Maybe? If the interrupt occurs after interruptSave is written, but before EIMSK is written, when the non-interrupt transaction completes last, it could end up restoring EIMSK with the ISR's saved copy.

Maybe something like this is needed? Any thoughts?

                        if (interruptMode == 1) {
                                uint8_t tmp = SREG;
                                cli();
                                interruptSave = SPI_AVR_EIMSK;
                                SPI_AVR_EIMSK &= ~interruptMask;
                                SREG = tmp;
                        } else

It could only if the ISR did an sei(); someplace before access.
I pass the magic 255 value to use the "block everybody' method, and it works almost as well as my spinlocks that I use in my xmem library.
The difference is that the spinlocks don't turn off interrupts for more than just a couple of instructions, where as the cli() idea just prevents all ISR from executing.

Since what I am doing on SPI tends to be brief (Max3421e USB Host Shield, using a real interrupt now!), I have not experienced any dropped serial characters or other funny business now that I have fixed this issue.

 

[PaulStoffregen]

Are you using an external interrupt pin with attachInterrupt()?

 

[PaulStoffregen]

For a time-frame, I'll get this merged on github either later today or early tomorrow.

This probably does merit an early 1.21 release. At this moment, no published library calls SPI.usingInterrupt() with any number greater than the external interrupt numbers.

Realistically, I don't see 1.21 happening before mid-November to early December. I am juggling a LOT of stuff and currently working on a couple high priority tasks that got delayed by the audio library.

 

[xxxajk]

Yes, I am using attachInterrupt, why?

 

[xxxajk]

Here is the relevant bits of code I am using to do the attachment, the settings for spi to do the transaction stuff are in the constructor...

...(SNIP!)...
        SPI.begin();
#endif

        SPI.usingInterrupt(255);

        pinMode(irq, INPUT);
        UHS_PIN_WRITE(irq, HIGH);
        pinMode(ss, OUTPUT);
        UHS_PIN_WRITE(ss, HIGH);

...(SNIP!)...


#if USB_HOST_SHIELD_USE_ISR
        // Attach ISR to service IRQ from MAX3421e
        // supports only pins 2 and 3 int0 and int1 for avr
        cli();
#ifdef __AVR__
        if(irq == 2) {
                ISReven = this;
                attachInterrupt(INT_FOR_PIN2, call_ISReven, LOW); // avr
        } else if(irq == 2) {
                ISRodd = this;
                attachInterrupt(INT_FOR_PIN3, call_ISRodd, LOW); // avr
        } else {
                sei();
                return (-2); // -2 means not able to assign ISR
        }
#else
        // Everybody else should be able to use any pin

        ....(SNIP!)....

#endif
        sei();
#endif

 

[xxxajk]

One thing that to this day kind of urks me is that there is no easy way to just say 'attachInterrupt(pin, handler, condition);' for _ALL_ boards, not just non-avr.
The avr ends up as a special case. This is silly.

I realize this is a bit of a rant, but I am wondering if we now need to push the arduino team into using a better interrupt scheme. Perhaps if done with Teensyduino as an example use-case, we can show that not only does it work, but that it can bring all libraries that use an interrupt to coexist together in harmony...

What should be done is that you do this by pin number, period. If an attempt to attach to a pin that can't generate an irq, it should return false, to indicate failure.
Failure should also include if it is the entire port, unless there is abstraction above it to handle it for you. I'm certain that this can be done transparently, since you can just mask off any pin that has a real direct IRQ.

I also know you (Paul) are a busy guy, and I'm willing to do the leg work on this sort of thing if you are willing to incorporate it into the next version of Teensyduino. It will totally help me because I will be able to release libraries that use an ISR and won't conflict. I already have one in the wild that does this, but it can only operate on an AVR due to this restriction, and I would love to add more libraries to my collection, and share them, but I can't without conflicts.

In conclusion, I do realize there are pinChangeInt libraries... it is a nice idea, but there are overlaps between the entire port and a single int in some cases, and I am not certain if those particular cases are handled correctly. It is also avr centric. Worse, no two libraries that use interrupt pins agree on what way to attach to these interrupts. I know I can do better than this, and it is a shame that the Arduino team did not have the foresight to do this properly.

Let me know if you are willing to entertain this idea, and then I can show the Arduino team how it can be done in a superior way, and hope that they will bite.

 

[xxxajk]

By the way, speaking of using/abusing cli();, wouldn't it have been better to disable other hardware interfering interrupts instead?
For example, to allow millis, serial, etc to continue to operate.
Yes I understand you can do this per-driver for pin interrupts. You can't do it for timers, or other internal hardware IRQ.
In the case of 'from a hardware IRQ' there should be a way to specify that, and add it to the list of things to disable. Timers are one such case.
If you do it that way, you don't need to use cli() to disable all ISRs, and any non-offending ISR can just hit as needed.

 

[xxxajk]

By the way, Just by looking at it, I think the arm versions also need atomic protection ;-) I've not tested this yet, but I will be doing so fairly soon

 

[xxxajk]

I'm reworking the SPI library, as it has a lot of problems and waste, all while keeping it compatible. Testing will be done with our super secret host library, and I'll share the source with you once it is done.
Most importantly, there are a pile of atomicity problems in many places.
I also have some additional good things to add. If you can hand this off upstream to the Arduino team that would be awesome.
It might be bundled with our library when it is released in any case, because I really don't like it when things go wrong, and I'm sure others do not either.

 

[PaulStoffregen]

I realize this is a bit of a rant, but I am wondering if we now need to push the arduino team into using a better interrupt scheme.

Actually, I pushed for this several months ago. Many people did support the idea of attachInterruptToPin(), but ultimately it was rejected.

But we did get a digitalPinToInterrupt() macro. It's officially released in 1.0.6 and 1.5.8.

#if USB_HOST_SHIELD_USE_ISR
        // Attach ISR to service IRQ from MAX3421e
        cli();
#ifdef digitalPinToInterrupt
        int n = digitalPinToInterrupt(irq);
        if (n == NOT_AN_INTERRUPT) return (-2);
        ISReven = this;
        attachInterrupt(n, call_ISReven, LOW);
#else
        // ugly fallback code for Arduino 1.0.5 & earlier...
        // supports only pins 2 and 3 int0 and int1 for avr
#endif
        sei();
#endif

The other nice thing about digitalPinToInterrupt() is it gives you the number to pass into SPI.usingInterrupt().

 

[PaulStoffregen]

Also, sorry about the delay. I've committed the fix to GitHub, so it's now queued for Teensyduino 1.21.

If any libraries or sketches are published with SPI.usingInterrupt(255), or if you need to release this new version of UHS, and end users end up hitting this problem, at the very least I'll push out 1.21-rc1 early to give people a path to get the fix easily installed.

 

[PaulStoffregen]

Regarding more fixes to the SPI library, yes, but please let me know what you're up to. Even if I can't respond rapidly, it'll be a lot easier than a huge pile of changes developed over many weeks, but I see them all at once.

I'm hoping you can try the code above, or something similar with digitalPinToInterrupt(), in your private UHS branch? I'm still a little concerned there might be a race condition in the case where SPI.usingInterrupt() gets a normal integer. Even if you stay with SPI.usingInterrupt(255), I hope you'll give it a try to see if the same problem happens?

The normal interrupt numbers are probably also a better solution for UHS to play nicely with other libraries. It avoids global interrupt disable, for much better compatibility with libraries like Servo and SoftwareSerial, which require very low interrupt latency.