dmadison / ServoInput

Interrupt-driven servo decoder library for Arduino
GNU Lesser General Public License v3.0
22 stars 10 forks source link

Interrupt related compilation issue with UNO R4 Minima #28

Closed Risto-H closed 9 months ago

Risto-H commented 9 months ago

When trying to compile code that uses ServoInput library - one of the examples for instance - the compilation fails with multiple error reports related to a missing interrupt capability. This happens even though the HW has interrupt capable pins D2 and D3 just like the earlier UNO versions.

The issue is easily reproduced by just selecting a code example accompanied with this library and compiling first with Arduino UNO selected. the switch to Arduino UNO R4 to see compilation fail.

This may be related to a similar issue found with ESP32. It could also have something to to with the interrupt handling of Renesas RA4 MCU.

dmadison commented 9 months ago

Hello, and thanks for creating this issue.

Please provide the following information:

Could you please also share the complete error message you're receiving?

Risto-H commented 9 months ago

All files (library & board files) and Arduino IDE are the latest available versions. I will check and list the exact versions later today and attach the compilation error messages as requested.

Risto-H commented 9 months ago

Version information:

Error messages: `In file included from C:\Users\risto\AppData\Local\Temp.arduinoIDE-unsaved2024018-5912-l6iyo9.fhl3\RC_Receiver\RC_Receiver.ino:31:0: C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h: In member function 'void ServoInputPin::attachInterrupt()': C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:109:49: error: 'NOT_AN_INTERRUPT' was not declared in this scope static_assert(digitalPinToInterrupt(Pin) != NOT_AN_INTERRUPT, "This pin does not support external interrupts!"); ^~~~ C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:109:49: note: suggested alternative: 'PIN_INTERRUPT' static_assert(digitalPinToInterrupt(Pin) != NOT_AN_INTERRUPT, "This pin does not support external interrupts!"); ^~~~ PIN_INTERRUPT C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h: In member function 'void ServoInputPin::detachInterrupt()': C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:125:38: error: 'NOT_AN_INTERRUPT' was not declared in this scope if (digitalPinToInterrupt(Pin) != NOT_AN_INTERRUPT) { // detach external interrupt ^~~~ C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:125:38: note: suggested alternative: 'PIN_INTERRUPT' if (digitalPinToInterrupt(Pin) != NOT_AN_INTERRUPT) { // detach external interrupt ^~~~ PIN_INTERRUPT

exit status 1

Compilation error: exit status 1`

dmadison commented 9 months ago

Thank you.

At the top of the header, try adding the line:

#define NOT_AN_INTERRUPT -1
Risto-H commented 9 months ago

I tried by adding the line to the RC_Receiver example which resulted into different error messages as before. I then realized that you probably meant Servoinput.h file and tried that as well. This resulted into the same errors as adding the line to the sketch.

Here's the error listing: `In file included from C:\Users\risto\AppData\Local\Temp.arduinoIDE-unsaved2024019-456-11b5vjf.8a95\RC_Receiver\RC_Receiver.ino:32:0: C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h: In instantiation of 'void ServoInputPin::attachInterrupt() [with unsigned char Pin = 2]': C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:100:18: required from 'ServoInputPin::ServoInputPin() [with unsigned char Pin = 2]' C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:103:62: required from 'ServoInputPin::ServoInputPin(uint16_t, uint16_t) [with unsigned char Pin = 2; uint16_t = short unsigned int]' C:\Users\risto\AppData\Local\Temp.arduinoIDE-unsaved2024019-456-11b5vjf.8a95\RC_Receiver\RC_Receiver.ino:46:77: required from here C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:110:5: error: non-constant condition for static assertion static_assert(digitalPinToInterrupt(Pin) != NOT_AN_INTERRUPT, "This pin does not support external interrupts!"); ^~~~~ C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:110:40: error: call to non-constexpr function 'pin_size_t digitalPinToInterrupt(pin_size_t)' static_assert(digitalPinToInterrupt(Pin) != NOT_AN_INTERRUPT, "This pin does not support external interrupts!");


C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h: In instantiation of 'void ServoInputPin<Pin>::attachInterrupt() [with unsigned char Pin = 3]':
C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:100:18:   required from 'ServoInputPin<Pin>::ServoInputPin() [with unsigned char Pin = 3]'
C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:103:62:   required from 'ServoInputPin<Pin>::ServoInputPin(uint16_t, uint16_t) [with unsigned char Pin = 3; uint16_t = short unsigned int]'
C:\Users\risto\AppData\Local\Temp\.arduinoIDE-unsaved2024019-456-11b5vjf.8a95\RC_Receiver\RC_Receiver.ino:53:77:   required from here
C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:110:5: error: non-constant condition for static assertion
     static_assert(digitalPinToInterrupt(Pin) != NOT_AN_INTERRUPT, "This pin does not support external interrupts!");
     ^~~~~~~~~~~~~
C:\Users\risto\Documents\Arduino\libraries\ServoInput\src/ServoInput.h:110:40: error: call to non-constexpr function 'pin_size_t digitalPinToInterrupt(pin_size_t)'
     static_assert(digitalPinToInterrupt(Pin) != NOT_AN_INTERRUPT, "This pin does not support external interrupts!");
                   ~~~~~~~~~~~~~~~~~~~~~^~~~~

exit status 1

Compilation error: exit status 1`

So it seems that the problem lies somewhere deeper.
dmadison commented 9 months ago

It looks to be an issue with inconsistencies between the Arduino cores. The renesas core does not implement the NOT_AN_INTERRUPT macro, and the digitalPinToInterrupt() function is not a compile-time constant expression like it is on other platforms.

I've pushed a patch to a new branch entitled "renesas" with a workaround. The examples compile on my machine, but I do not have an Uno R4 on hand to test. Please give the branch a test and report back.

Risto-H commented 9 months ago

I tested quickly with the "Calibration" example since it needs only one input pin. It compiles, uploads and runs OK but does not seem to detect the servo signal. It keeps printing "Waiting for servo signal...".

I will build a better cable to use with a servo tester tomorrow and verify but my current connection should work as well. Servo tester powers up from +5 V and GND pins on UNO R4 and signal wire is connected with a test hook to D2. Thus the connection is the same as you show in your article but uses a servo tester instead of an RX.

Could I add some lines to the code to help finding if interrupts are actually generated? Meanwhile, I'll try the standard pulsein to make sure my setup works otherwise.

dmadison commented 9 months ago

Unless the driver is sending a signal out of spec, the available() function should be enough to tell you that the ISR is firing.

Here's two things to try to narrow down the issue.

First, try with everything defined in the setup() function, instead of split between global/setup/loop:

void setup() {
    Serial.begin(115200);
    ServoInputPin<2> servo;

    while(true) {
        float angle = servo.getAngle();  // get angle of servo (0 - 180)
        Serial.println(angle);
    }
}
void loop() {}

Second, try using a helper function to call the ISR:

ServoInputPin<2> servo;

void helperISR() {
    servo.isr();
}

void setup() {
    Serial.begin(115200);
    attachInterrupt(digitalPinToInterrupt(2), helperISR, CHANGE);
}

void loop() {
    float angle = servo.getAngle();  // get angle of servo (0 - 180)
    Serial.println(angle);
}
Risto-H commented 9 months ago

Thanks for these!

Both of these test codes work. ISR is firing and I get data from 0 to 180 degrees when I adjust the servo tester between the extremes. However, the readings are quite noisy, angle accuracy seems to be roughly +/-5 %. I know for sure that the servo tester outputs very clean pulse with 1 us accuracy and low jitter. I verified it with an oscilloscope.

I don't know if the noise is a problem at this point since the idea was to see if interrupts are being generated or not.

dmadison commented 9 months ago

Thanks for following up. If both of those codes work it confirms that it's an issue with the class constructor attaching the interrupt. I'm not sure whether that's because of a static initialization order issue or because the chip's interrupt support doesn't spool up until main(), but the solution either way is to remove the auto-interrupt hook and make the user do it themselves. That's a breaking change and will require a new version of the library. Grr.

Re: noise, are you saying accuracy is +/-5% of the dialed angle, or that the reported angle jitters +/-5% when the driver's angle is supposedly constant?

Please try recording the output using available() and the getPulseRaw() function and report back:

if(servo.available()) {
    Serial.println(servo.getPulseRaw());
}

If you have access to a scope and the means to do so, please attach images of the waveform when connected in situ to the Arduino.

Risto-H commented 9 months ago

The reported angle jitters by roughly +/- 5 % of full scale when the angle (pulse width) is kept constant. In other words the jitter is about +/- 9 degrees regardless of what the servo tester setting is. Sorry that my statement was a bit unclear.

I'll continue testing tomorrow, hopefully. I have a small electronics lab at home so taking scope images is no problem at all.

Risto-H commented 9 months ago

OK, I did a quick test already. I added the new code to the loop of the 2nd test code and commented the servo.getAngle related lines out.

The raw measurement is much much better for some reason. When "middle point" is selected from the servo tester, it outputs exactly 1520 us pulse which is what Futaba RC systems do. The new code measures it quite well:

image

I can verify the pulse width and shape soon with my oscilloscope. The measurement noise seems to be just +/-3 us (varies a bit more than is visible in this plot) with a +2 us offset. The offset error is either caused by the inaccurate clock of the MCU since it uses its internal oscillator or pulse shape or both.

dmadison commented 9 months ago

Interesting. Try using the original code, but with the available() block and see if you get the same jitter you had before:

if(servo.available()) {
    float angle = servo.getAngle();  // get angle of servo (0 - 180)
    Serial.println(angle);
}
Risto-H commented 9 months ago

This approach works better and with the available() block the angle output reports 93.2 deg - 94.5 deg. I observed this over a longer period and there's definitely variation due to the temperature correction that adjusts the internal oscillator of the MCU. It looks like now angle reporting is in line with the getPulseRaw() reporting in the previous test.

BTW, here's an oscilloscope image of the servo tester output in middle position. Persistence is 10 s here and no jitter to be seen:

Servo tester neutral output

Risto-H commented 9 months ago

I forgot to mention one detail that might help in troubleshooting: When the 2nd test code with

void loop() {
    float angle = servo.getAngle();  // get angle of servo (0 - 180)
    Serial.println(angle);
}

is installed, it is difficult to get another sketch uploaded. Double tapping reset button seems to be the only remedy. To me this sounds like the interrupt is firing at least partially randomly and even removing the servo signal does not help.

dmadison commented 9 months ago

Do you have the same issues uploading if you remove the Serial.println statement?

Risto-H commented 9 months ago

No, if that line is commented out, another sketch can be uploaded normally without messing with the reset button. I just tried that once but the behavior was quite consistent earlier. I had to do some research to find out the fix...

dmadison commented 9 months ago

It sounds to me like the library is functioning properly, and the cause of the jitter is from the serial implementation blocking or otherwise delaying the interrupt from firing. It might be prudent to add the available() line to the library examples for platforms where issues like that exist.

Other than that, is the library working properly? I can merge the renesas branch and then try to find some time to work on the interrupt setup issue.

Risto-H commented 9 months ago

I tend to agree - even though I have very little or no coding skills, I do know the hardware side and how interrupts function.

Based on the results with available() block, the library indeed works properly and most of the remaining measurement uncertainty is most probably caused by the internal clock source of the MCU. More testing is of course needed to find out but it looks promising. I might even test the accuracy with crystal source installed later since I found that there are methods to activate it once the necessary parts (that I have) have been added.

I will go on and try to create the simple LED control system for F3P model airplanes that I was originally planning to do. The servo pulse decoding part does not have to be super fancy so the standard Arduino pulseIn() would probably have been sufficient but being a nerd lead me into testing this library with the R4... :)

dmadison commented 9 months ago

I've pushed some changes to a "v2" branch which should hopefully solve the interrupt issue. The new version of the library requires a call to servo.attach() in setup(). Please give it a try and let me know if you have any problems.

Risto-H commented 9 months ago

I tested this v2 branch with two of the examples: BasicAngle and Calibration. The library works OK but with Uno R4 the examples need the additional servo.available() block to work properly (as you already said above). Without it, the sketch can read values but they are very jittery (just like before with the tests). Alternatively a small delay in the loop helps as well and gives the same accurate results. In the case of my servo tester, the positive pulse is repeated at 67 Hz and thus 20 ms delay is appropriate. I tested with 10 ms delay and while that works much better than without it, the output displays high spikes occasionally. Probably the interrupts clash with each other somehow as you say, even though pin D2 is associated with IRQ0 that has the highest priority of normal pin interrupts.

GetAngle example change:

void loop() {
if (servo.available()) {
  float angle = servo.getAngle();  // get angle of servo (0 - 180)
    Serial.println(angle);
   // delay(20);
  }
}
dmadison commented 9 months ago

That sounds about right, given the current assumption that it's an issue with Serial delaying the interrupt. Since you have access to a scope, try something like this:

const unsigned long Center = 1520;
const unsigned long Window = 50;
const int AlertPin = 3;  // or whatever pin is easy

void loop() {
    const unsigned long raw = servo.getPulseRaw();
    if(raw <= (Center - Window/2) || raw >= (Center + Window/2)) {
        digitalWrite(AlertPin, HIGH);
        digitalWrite(AlertPin, LOW);
    }
}

Scope the AlertPin alongside the servo signal and see if it fires. If it doesn't, that would confirm that the Serial implementation is the culprit. If it does, the problems may be in another castle.


For what it's worth, I took a peek into the 'renesas' core and the interrupt request table is set up in main() before the Arduino functions are invoked. That explains why the v1 library with the auto-attached interrupt did not work.

Risto-H commented 9 months ago

Thanks, I'll test that during the weekend (or even earlier) and report back. It's pretty clear to me what that code does.

Risto-H commented 9 months ago

I tested the "AlertPin" approach with this code:

ServoInputPin<2> servo;
const unsigned long Center = 1520;
const unsigned long Window = 50;
const int AlertPin = 3;  // or whatever pin is easy

void setup() {
  servo.attach();  // attaches the servo input interrupt
  pinMode(AlertPin, OUTPUT);
}

void loop() {
  const unsigned long raw = servo.getPulseRaw();
  if (raw <= (Center - Window / 2) || raw >= (Center + Window / 2)) {
    digitalWrite(AlertPin, HIGH);
    delay(1);
    digitalWrite(AlertPin, LOW);
  }
}

It took me a while to figure out that the pin must be defined as OUTPUT first but once I added that, the code works and AlertPin stops changing state when the pulse width is inside the Window. Thus nothing in this code causes jitter in the measurement. When pulse width is outside the Window, the pin changes state all the time very rapidly. I added 1 ms delay to be able to see the signals properly in this case.

dmadison commented 9 months ago

If that's the case, the issue must either be in the Serial implementation, or from an unexpected interaction between the library and the Serial implementation.

I'm hesitant to add the if(servo.available()) code block to all examples to mitigate this, since it's been my experience that users will often copy the example code verbatim without understanding what it does. That will lead to blocking issues as the library waits for the next servo pulse, which is the same behavior as pulseIn() (and what this library was written to avoid).

I also haven't experienced the jittering issue on any of the other platforms I've tested, which leads me to believe it's a fault with the R4 core implementation.


Otherwise, it sounds like the library is working as well as can be expected on the Uno R4. It now compiles without error and the ISR attaches as designed. I'm going to merge the v2 branch, push a release, and close this issue. Thank you for all of your help and feedback!

dmadison commented 9 months ago

Resolved by #30.

Risto-H commented 9 months ago

Thank you very much for the efforts.

I agree with your reasoning. It wouldn't make sense to add the if(servo.available()) block to the examples except maybe in comment form. Also the R4 USB serial implementation is the likely culprit. I'll try (remember) to test if the same happens with a traditional serial UART as well.

Would it be possible to report this behavior to the R4 core team as a bug?

dmadison commented 9 months ago

Yes, you can create an issue on the Arduino renesas core: https://github.com/arduino/ArduinoCore-renesas

Although for their benefit it would be better if you could create a minimally reproducible example that does not involve the library. Perhaps an interrupt function that records the value of micros() on a rising edge and prints the time between triggers? Something like:

const int pin = 2;
volatile unsigned long m;
unsigned long last;

void isr() {
    // record time on rising edge
    if(digitalRead(pin) == HIGH) m = micros();
}

void setup() {
    Serial.begin(115200);
    pinMode(pin, INPUT_PULLUP);
    attachInterrupt(digitalPinToInterrupt(pin), ISR, CHANGE);
}

void loop() {
    const unsigned long x = m;
    Serial.println(x - last);
    last = x;
}

Completely untested, but that's the gist.