digitalPinToInterrupt possibly broken in 1.4.7

[dpetican]

This program works in core 1.3.6 In 1.4.7 all I get are nonsense values like the program is reading a floating pin. The count (not flow) is high like 16K plus so I figure that must be high frequency noise on an unconnected pin. I verified the proper signal is present on pin 20 using a function generator and scope. Strictly speaking the drift calc is only required if I use delay() in the main program which I don't. I'm using AVR128DB28. Thanks.

I don't think the problem is the macro, but what is the point of it anyway because in 1.3.6 I can just put in the pin number without the macro and get the expected result? BTW I am actually using this for a flow sensor with an external pull-up. 0.45 is the flow sensor constant.

`#define PCINTFT1 20

volatile unsigned long count = 0; float flow = 0.0;

static void countPulses(void) { count++; }

void readFlow(void) { static unsigned long startTime = millis();

unsigned long now = millis(); unsigned long timePassed = now - startTime; if (timePassed > 1000UL) { noInterrupts(); flow = (float) count; count = 0; interrupts(); startTime = millis(); float drift = 1000.0 / (float) timePassed; //Convert pulses to L/min (time spent counting pulses is always slightly more than 1 sec) flow = flow 0.45 60.0 / 1000.0 * drift; } }

void setup() { Serial.begin(115200); attachInterrupt(digitalPinToInterrupt(PCINTFT1), countPulses, RISING); }

void loop() { readFlow(); Serial.println(flow); }`

[SpenceKonde]

The reason for the interrupt is that there is existing code that assumes that it will need it, and people would complain. The problem is not so much user code, but library code being used by people who are not yet experienced enough as programmers to understand the how the library works or what to do to fix it.

The macro is just defined as

#define digitalPinToInterrupt(P)            (P)

1.4.0 introduced a COMPLETE reimplementation of attachInterrupt() - I thought that I had gotten all the gremlins out by now though, so I'd very much like to figure out what's going on here.

Right now there are three options in the tools -> attachInterrupt mode menu - the default is to use the new implementation, but automatically enable it on every port. This has little benefit in terms of speed, but it does save about 40 bytes per port after tje first, and for interrupts with low numbers within each port, it is more compact (stock implementation is (handler overhead)+(save+restore overhead - 30-40 bytes or so?) (number of ports), whereas now it's (handler overhead + save+restore overhead) + 6 number of ports.

The real motivation though was the second option, manual mode - where you have to call attachAenable() or something like that to enable interrupts on portA for example (refer to the interrupt reference for the actual detailes) before you use attachInterrupt). In this mode, you can have attachInterrupt interrupts on some ports, coexisting with manually defined interrupts on another port. AttachInterrupt othertwise will grab every single pin interrupt vector for itself when it is rferenced anywhere in the code or by an included library, so any manually defined pin interrupt would generate a duplicate vector error., Among other tjoigns, this meant that "SoftwareSerial" would keep you from manually defining interrupts, ypou could only do bulky and slow interrupts used through attachInterrupt. Manually defined interrupts which refrain from calling any function that can't be inlined will always be far more performant than any "attachInterrupt" type of deal, because if it knows what code it's running, and the code is well written, it won't have to push a ton of registers onto the stack at the beginning and then pop them off at the end, and there are many use cases that are totally viable with manually defined interrupts, buit hopeless with attachInterrupt.

The third option switched back to the old version of attachInterrupt in the event that there were problems with the new one (I've fixed several of them) Try that option to confirm the nature of the problem.

Not too long ago there was also a bug with pins 0 and 1 on each port on the 28-pin DA and DB parts due to a copy-paste error in pins_arduino.. I think that came after 3.6 though,

[dpetican]

Okay I see the menu option now in light of the way you explained it. Remember I'm coming from 3.x to 4.x. That menu was not there in 3.x so when I saw the phrase "as usual" I assumed that was the way it was done on 3.x. What else could I think? I will try this tomorrow maybe when I am in front of the board.

In the menu option you say after the old version, "new one is worse". So why is the old version not the default? Well maybe it was and I changed it for "as usual". I don't remember.

As a complete aside why do you care so much about memory? I write highly complex and large programs and never fill 128k of memory. As you correctly pointed out the 64k and 32k parts are somewhat to non-existant. Even the price difference is small. if someone needs to save $0.50/board on a million part production run they are using a different tool chain I should hope. In fact 50% of my programs are string constants stored in flash. Throwing in the kitchen sink never works IMHO.

[SpenceKonde]

I will correct the issue with the text of the menu. I do not know where those words came from - it's supposed to say something like "Old version, (may fix bugs)" like it does in megaTinyCore.

I care about memory because this codebase is used for both DxCore and megaTinyCore. Some pincounts on megaTinyCore are limited to only 4k of flash. They are either very popular, or their users are very loud.. Every month or so someone on a 412 (the largest 8-pin modern avr!) complains about how something takes 50 or 100 bytes more in a new release or how something is so bloated and somesuch, and now their carefully optimized code doesn't fit anymore. There are more users on megaTinyCore than DxCore, by a significant margin, (though DxCore is catching up), so if I have to choose between a solution that's great for tinies but uninspiring for the Dx, vs a great solution for DX that is bloated on tinies, I will pick the tiny-friendly solution, since I do not have time to maintain multiple implementation of most of the core code. The places where there are big differences in the implementations are places where there was a lot of bang for the buck = Like the DxCore feature that let you move around the PWM to almost any pin on the chip (most of the code is written to do this for TCD0 PWM when we get the fix for the alternate mappings too - it's impractical on te tinyAVRs due to the combination of a different implementation in hardware and more limited resources), or the internal oscillator tuning on the tinies, which was only practical because of the insanely huge compliance of the internal oscillator on the modern tinyAVRs (the difference between whatever name the analog of OSCCAL is called on modern tinies being set to 0 and set to 0x3F (0/1-series_ or 0x7F (2-series) is approximately equal to the nominal speed of that oscillator option. On the DX, it's way the fuck less, like +/- 8% or something useless like that) - or where it was literally forced on me - USERSIG library has to be different, because tiny's get byte erase granularity but Dx gets no erase granularity. Writing the flash is also totally different (modern tinyavr writes by page, Dx only erases by page, but writes by the word or byte - hence that library is different). The libraries for Event, Comparator, Logic, EEPROM, tinyNeoPixel.... those are all the same code on the two cores - there are ifdefs, sometimes a lot of them - but it's critical to being able to not leave one core or the other behind to have a single file to support all the parts. When I was forking libraries to adapt them to one core or another, I'd come back in 6 months, realize that the library on the less-recently-updated core was missing all kinds of features I needed, and it was awful.

So yeah - I care about memory because I'm writing the code for parts with 4k of flash in mind. Don't forget, the next three product lines in the Microchip pipeline all cap out at 64k, of flash, max, and go down to 16k (DD, DU - and the exciting DU parts, the 14-pin and 20-pin ones, were only mentioned in the 2 smaller flash sizes) or even 8k (EA-series, which will be the first full-size chips with the new insanely great ADC). There's no mention of any more 128k parts, much less anything larger, and with the chip shortage looking likely to persist for years, we have to take what we can get for sizes; Right now the 32k tinyAVRs are abundant, but in a few months, they're probably going toe be backordered for 6 months+ and people will be having to make do with the smaller ones.

Though let me say again, the main motivation of this was NOT flash or memory. The benefit was that it provided a way to use a library that relied on attachInterrupt and a manually defined pin interrupts in the same sketch. There are a great many libraries that use attachInterrupt. On clasic AVRs this is fine, because that only covers INT0/INT1 type interrupts, rather than PCINTs. On modern AVRs, there's only one kind of pin interrupt, and using

a well written ("just set a flag or increment a counter") can execute and have returned to the code it was called from before an attachInterrupt interrupt even reaches the user code. Recently I had a discussion with someone where I went through the math to show that that their code could never work, because it couldn't meet the nyquest sampling criteria for the signal they were trying to measure... And one of the most common uses of an interrupt doesn't need ANY code in the ISR. it just needs an empty interrupt to wake the chip up; it offends my sensibilities to have it go through 100+ clock cycles of overhead when I only need one instruction, 'reti' (return from interrupt), to resume execution at the point where I put the chip to sleep.

[dpetican]

Yes the third option fixes the problem. Thanks.

[SpenceKonde]

There is a fix now available in github, if you could verify that it fixes the problem that would be very helpful, I don't have a test setup that reoproduced the bug.

It is sufficient to replace just Winterrupts.c

Once someone either here or on megaTinyCore has tested this and reported it to work, releases for both cores will be pushed.

[SpenceKonde]

This should be fixed by the attachInterrupt fix (it;s attachInterrupt that was broken)