Calling attachInterrupt(pin, ISR) within a c++ objects constructor

[KurtE]

Not sure if best to post here and/or as a Github issue:

I did open this up as an issue a couple of days ago:
calling attachInterrupt(pin, ISR) within a c++ objects constructor · Issue #56 · arduino/ArduinoCore-renesas (github.com)

And mentioned in a different thread about the Encoder library.

Simply put: any class that uses the attachInterrupt in its constructor, will fail for any of these objects that are global.

I have a simple example sketch, that shows it:

#define USE_ONESTEP_CONSTRUCTOR

class test_stuff_in_constructor {
public:
  void begin(uint8_t pin) {
    if (_pin != 0xff) {
      Serial.println("begin: was previously called");
      return;
    }
    _pin = pin;
    pinMode(pin, INPUT_PULLUP);
    attachInterrupt(pin, &changeFunc, CHANGE);
    pinMode(LED_BUILTIN, OUTPUT);
    _counter = 0;
  }

  test_stuff_in_constructor() {}

  test_stuff_in_constructor(uint8_t pin) {
    begin(pin);
  }

  uint32_t count() {
    return _counter;
  }
  uint8_t pin() {
    return _pin;
  }
private:
  uint8_t _pin = 0xff;

  static volatile uint32_t _counter;
  static void changeFunc() {
    digitalWrite(LED_BUILTIN, HIGH);
    _counter++;
  }
};

#ifdef USE_ONESTEP_CONSTRUCTOR
test_stuff_in_constructor testobj(2);
#else
test_stuff_in_constructor testobj;
#endif

volatile uint32_t test_stuff_in_constructor::_counter = 0;

extern void DumpIRQTable();

void setup() {
  while (!Serial && millis() < 4000)
    ;
  Serial.begin(115200);
  Serial.println("Test constructor stuff");
  DumpIRQTable();

#ifndef USE_ONESTEP_CONSTRUCTOR
  testobj.begin(2);
  DumpIRQTable();
#endif  


  Serial.print("Pin: ");
  Serial.println(testobj.pin());
}

uint32_t previous_count = 0;
void loop() {
  // put your main code here, to run repeatedly
  uint32_t count = testobj.count();
  if (count != previous_count) {
    Serial.println(count, DEC);
    previous_count = count;
  }
}

Note: I am in process of trying to localize the issue. And I have added some instrumentation code
to the attachInterrupt (interrupts.cpp). Saved away stuff, and add a print debug information, that I can call later...
(Don't think I can call Serial.print() in constructor, and hardware debug not not implemented? yet.

//////////////////////////////
pin_size_t lai_pinNumber;
voidFuncPtrParam lai_func;
PinStatus lai_mode;
void* lai_param;
CIrq *lai_irq_context;
int lai_ch;

////////////////////////

void DumpIRQTable() {
    Serial.println("Dump IRQ Table");
    Serial.print(lai_pinNumber, DEC); Serial.print(" ");
    Serial.print((uint32_t)lai_func, HEX); Serial.print(" ");
    Serial.print(lai_mode, DEC); Serial.print(" ");
    Serial.print((uint32_t)lai_param, HEX); Serial.print(" ");
    Serial.print(lai_ch, DEC); Serial.print(" ");
    Serial.println((uint32_t)lai_irq_context, HEX);

    for (int i = 0; i < MAX_IRQ_CHANNEL; i++) {
        Serial.print(i, DEC);
        CIrq *pcirq = IrqChannel.get(i, false);
        if (pcirq) {
            Serial.print(" 0x");
            Serial.print((uint32_t)pcirq->fnc_void, HEX);
            Serial.print(" ");
            Serial.println(pcirq->cfg.irq);

        } else {
            Serial.println(" <None>");
        }
    }
}

Mucked up attach:

/* -------------------------------------------------------------------------- */
void attachInterruptParam(pin_size_t pinNumber, voidFuncPtrParam func, PinStatus mode, void* param) {
/* -------------------------------------------------------------------------- */    
    lai_pinNumber = pinNumber;
    lai_func = func;
    lai_mode = mode;
    lai_param = param;


    CIrq *irq_context = nullptr;
    int ch = pin2IrqChannel(pinNumber);
    
    if(ch >= 0 && ch < MAX_IRQ_CHANNEL) {
        irq_context = IrqChannel.get(ch,true);
    }
    lai_ch = ch;
    lai_irq_context = irq_context;
...

So if I call it in the constructor:

Test constructor stuff
Dump IRQ Table
2 4101 2 0 1 20000A30
0 <None>
1 0x0 0
2 <None>
3 <None>
4 <None>
5 <None>
6 <None>
7 <None>
8 <None>
9 <None>
10 <None>
11 <None>
12 <None>
13 <None>
14 <None>
Pin: 2

And the ISR is never fired.

Changing to the two step initialization and things work.

Test constructor stuff
Dump IRQ Table
0 0 0 0 0 0
0 <None>
1 <None>
2 <None>
3 <None>
4 <None>
5 <None>
6 <None>
7 <None>
8 <None>
9 <None>
10 <None>
11 <None>
12 <None>
13 <None>
14 <None>
Dump IRQ Table
2 4101 2 0 1 20000A58
0 <None>
1 0x4101 5
2 <None>
3 <None>
4 <None>
5 <None>
6 <None>
7 <None>
8 <None>
9 <None>
10 <None>
11 <None>
12 <None>
13 <None>
14 <None>
Pin: 2

And the ISR is called...

It almost looks like the code logically called detachInterrupt() or ... I don't think it called the IrqPool destructor as it would have cleared the memory allocation so would not shown anything in slot 1.

Also could be some other c++ constructor called later that messed it up.

Still debugging

[6v6gt]

In principle, you should avoid having any dependencies on the state of the Arduino environment in a constructor function. This is because the constructor may be invoked before the Arduino environment has been set up and some configuration may be later clobbered during this process. In the case of attachInterrupt(), the Arduino environment takes over all possible interrupt handles so the user can then specify an own call back routine.

[b707]

It is an absolutely expected behaviour, not an issue, and nothing especial for Uno R4. I have faced with the same problem in STM32 controller, for example.

As a general rule you shouldn't call a hardware configuration methods (pinMode(), attachInterrupt(), etc) in class constructors, because initialization of global objects can occurs before initialization of the controller hardware.
In such cases you must define a dedicated method in your class to hardware init, like begin() and call it in setup()

[KurtE]

@6v6gt

In principle, you should avoid having any dependencies on the state of the Arduino environment in a constructor function.

@b707

It is an absolutely expected behaviour, not an issue, and nothing especial for Uno R4. I have faced with the same problem in STM32 controller, for example.

I both agree and disagree.

In general, I partially agree, and I typically will write my libraries as such to use a two step initialization.

However, there are lots of libraries out there that don't. And the real question to me, when some company comes out with a new board, especially Arduino and especially if they call it as a revision of an existing board, one should strive to make the code as compatible as possible with the other releases of the board. Which at times can be an interesting problem.

In this case, for example the Encoder library did it as a one step...
And worked fine on a lot of different boards, like: AVR based boards, Leonardo, Chipkit, ESP3286, ESP32, Teensy, ...

In this case now, we have a PR back to Encoder library to allow for two step, which may or may not ever be pulled in. On some other PRs due to compatibility differences with this board, some of them that I submitted probably won't be.

But currently just trying to understand why this is not working...

[KurtE]

@Delta_G

They are, at least on this topic, in lock step with one another. The two step initialization has always been the recommended idea. The fact that it accidentally worked in a lot of cases and that got abused by people writing outside libraries is in no way the fault of the people at Arduino.

I understand what you are saying and don't disagree.

But: if you end up with tons of such issues with this board or boards and there are other boards and their core software handles such things in a better way, I would tend to put my energy into those other platforms.

You are right that there is now, some documentation on writing libraries that mention the two step initialization, and some of the reasons they mention is that the system might not be initialized when your constructor is called. But is only partially true. That is your system code is in control of when c++ constructors are called. For example, If you look at the core code for a Teensy such as the T4.x.
cores/teensy4/startup.c at master · PaulStoffregen/cores (github.com)
You will see that the c++ constructors are called pretty late in the game, as to handle some of these cases.

Yes, you still run into the issue that you don't have much control on the order that the constructors are called.

In this case you can avoid some of these issues, if you can get a lot of your lower level c++ objects, to have their constructors use constexpr and have everything initialized such that the object is simply loaded and nothing needs to be called.

But at this point, I will probably take the simple route and punt...

[KurtE]

Thanks everyone for the input,

For now Punt.

While I believe in trying to improve compatibility, it is Arduino's decision on how compatible and robust they wish for the platform.

[6v6gt]

@KurtE

it is Arduino's decision on how compatible and robust they wish for the platform.

Not exclusively. On some platforms, the Arduino core is heavily dependent on upstream code from the manufacturer so if a new version that appears with breaking changes e.g. dateTime datatype now 8 bytes instead of 4 bytes then so be it. The manufacturer has decided.

[KurtE]

@Delta_G @6volt42 - Sorry I thought was giving a pretty simple generic response, on saying it is up to Arduino to decide...

And as I mentioned, I personally prefer the two-step initialization. And also know what a pain it can be to try to support the one step. In the current Teensy beta version (0.59.2), one of the main reasons for this cycle is to test to change: -std=gnu++17
I believe the current released version is 14.

And it has uncovered some issues, which broke some external libraries, who rely on the SPI library in their constructor. A fix has been checked in to the SPI library to resolve this. And I expect there will be another beta release soon. That was PJRC's choice in this case.

@6v6gt

Not exclusively. On some platforms, the Arduino core is heavily dependent on upstream code from the manufacturer ...

Total agreement. i.e. Arduino's choice...

@Delta_G

What would you have them do? Write their own compiler to ensure that all undefined behavior in C++ is defined?

Of course not.

Simply saying it is Arduino's choice, on when to change to a different version of the GCC compiler, or which compiler flags, which c++ standard, etc.

And when they decide to change such things, how much testing is done to see the impact on those changes. Is there some form of beta cycle? And when differences are found, what if anything to do about it.

And one very valid option might be to punt on it. Preferably documenting it somewhere.

Which was why from your comments that it was appriate marked this one as punt... I also put comment in Issue saying feel free to close.

Thanks again.

[b707]

@KurtE

Simply saying it is Arduino's choice, on when to change to a different version of the GCC compiler, or which compiler flags, which c++ standard, etc.

You are very much mistaken. In most cases, there is no such choice. Arduino does not develop compilers, but takes those that exist for a given architecture. For most controllers, there are one or two toolchains with hard-coded compiler versions and available libraries.

@KurtE
What they're trying to explain to you is that one-stage initialization, as you call it, is simply erroneous code. And dragging it into new controllers for the sake of compatibility is not the best solution.

[KurtE]

@ptillisch and other moderator- Maybe best to kill this thread?

Sorry I know this is off topic, but I am trying to decide, how best to answer here and if it is worth investing my time.

Probably for now I will go back to working on the Teensy USB Host code and figure out why it does not like the USB Conversations from Arduino boards like Leonardo that are using the Mouse and HID libraries. Then maybe back to some other projects. Let me know if you are interested in the UART write code that I issued a Draft PR on. There is more that can and should be done on it, but not sure if anyone is interested.

But I will try to answer some of the comments from the last couple of responses.

@b707

You are very much mistaken. In most cases, there is no such choice. Arduino does not develop compilers, but takes those that exist for a given architecture. For most controllers, there are one or two toolchains with hard-coded compiler versions and available libraries.

Sorry I am assuming that most boards are using the GCC toolset. And probably ones installed under arduino/tools/arm-none-eabi-gcc. And yes there is a finite set of releases. This is true for most every other set of boards as well.

@b707

What they're trying to explain to you is that one-stage initialization, as you call it, is simply erroneous code. And dragging it into new controllers for the sake of compatibility is not the best solution.

Sorry I disagree with the term erroneous here. It is valid to say that Arduino does not support it. Remember in this case the class code is simply calling an API....

Is it worth the effort to make it work? hard to say. Not my call, other than to decide if I wish to investigate it any farther.

@Delta_G

About undefined behavior? How? How exactly can they anticipate every possible dumbass thing that third party library writers might do.

They usually test all the core libraries and make sure that they work. But how can they possibly be responsible for the hundreds and thousands of pieces of contributed code?

They cant! At least not alone. I am assuming that is what the beta period for the board was to help do. And I was assuming that would be continuing now.

On some previous new boards, some of us developers and other beta testers, generated a list of what we thought at the time were some of the more commonly used libraries. Each of us tested some of them, and we would try to keep a list of those, that worked, those, that did not and why not. We would then try to update this list, when we either created Pull Requests for our code and/or Pull Request calls to external libraries, or that we would not fix.

An example of this was the Teensy 4 beta. And you can see some of this list on the first page of the forum posting:
Teensy 4.0 First Beta Test (pjrc.com)
Were we perfect... NO. Are there still issues yes.

But that is how I would (and we have) done it.

@Delta_G

Please, tell me how exactly you think they could maintain compatibility there.
...
At least propose it instead of just insinuating that someone else had the ability to do something that they don't.

Could they have used the same compiler and toolchain? Would it have worked? What other solution are you proposing? Or are you just on about something that you don't really understand.

I don't have a solution yet, I started this thread, as a place to hopefully discuss the issue with others who are also trying to improve the core code and who might have some insights on things.

like in Interrupts.cpp the main code is in the attachInterruptParam function, which is pretty straight forward, in that it maps the pin number to irq channel, and the tries to retrieve a CIrq object from the IRQPool object. Both of these C++ objects are defined in this file. the IrqPool object constructor does nothing. The code will do a new CIrq...

I have debug sprinkled in here which looks like it is storing the data out, but when we get to setup() the IrqPool is still pointing to the same object for this item, but the data is not there...
So where was it wiped out? Was trying to locate.

This has very little to do with which version of the GCC compiler or the like. Maybe it has to do with when are the c++ objects initialized? Probably buried somewhere within the FSP.
Can some of the Arduino code be initialized before this? i.e. are there hooks built in?

But for now I have lost interest.