Fix build for UnoR4 Wifi/Minima

[pgrawehr]

Build was broken possibly due to a change in the
core libraries.

[per1234]

Although this might allow the library to compile, it is not correct.

This change causes the "Arduino UNO R4 Boards" core's IS_PIN_PWM macro to be used.

This library's IS_PIN_PWM macro function takes an Arduino pin number argument and returns a Boolean value according to whether the pin has a PWM capability.

The "Arduino UNO R4 Boards" core's IS_PIN_PWM macro takes a low level pin configuration data argument, not an Arduino pin number. The macro that takes the Arduino pin number is digitalPinHasPWM. You can see the implementation here:

https://github.com/arduino/ArduinoCore-renesas/blob/1.2.0/variants/UNOWIFIR4/pins_arduino.h#L93-L93

#define digitalPinHasPWM(p) (IS_PIN_PWM(getPinCfgs(p, PIN_CFG_REQ_PWM)[0]))

(note that it passes the Arduino pin number argument to getPinCfgs before passing the return value of getPinCfgs to IS_PIN_PWM)

So if the library passes the macro an Arduino pin number argument, you will not get the correct result.

You can check it by running this sketch:

void setup() {
  Serial.begin(9600);
  delay(1000);

  Serial.println("| Arduino Pin | digitalPinHasPWM | IS_PIN_PWM |");
  Serial.println("|-|-|-|");

  for (pin_size_t pin = 0; pin < NUM_DIGITAL_PINS; pin++) {
    Serial.print("|");
    Serial.print(pin);
    Serial.print("|");
    Serial.print(digitalPinHasPWM(pin) ? "true" : "false");
    Serial.print("|");
    Serial.print(IS_PIN_PWM(pin) ? "true" : "false");
    Serial.println("|");
  }
}
void loop() {}

Result:

Arduino Pin	digitalPinHasPWM	IS_PIN_PWM
0	true	false
1	true	false
2	true	false
3	true	false
4	true	false
5	true	false
6	true	false
7	true	false
8	true	false
9	true	false
10	true	false
11	true	false
12	true	false
13	true	false
14	false	false
15	false	false
16	false	false
17	false	false
18	true	true
19	true	true

[pgrawehr]

That's bad.

Do you have any suggestion? The previous implementation didn't work either, because digitalPinHasPWM is also a macro, requiring the original implementation of IS_PIN_PWM.

As noted in the issue you reported, one solution would be to rename the macro troughout the library, but that would be a bit ugly and particularly risk that other boards will be broken.

[per1234]

one solution would be to rename the macro troughout the library, but that would be a bit ugly and particularly risk that other boards will be broken.

What is the risk? Unfortunately I'm not familiar with this library. Is this macro intended to be part of the library's public API, or is it only intended for internal use by the library's codebase?

Assuming the macro isn't intended to be part of the public API, I don't see anything ugly about the rename. It is best practices to add a "namespace" prefix to any macro in the global namespace (after all, if Arduino had done that then this problem wouldn't have occurred). So I would say this change could be better described as "beautiful" rather than "ugly".

Do you have any suggestion?

The only other solution I can think of is to duplicate the code from the "Arduino R4 Boards" platform's core:

--- a/src/utility/Boards.h
+++ b/src/utility/Boards.h
@@ -814,7 +814,9 @@ static inline void attachInterrupt(pin_size_t interruptNumber, voidFuncPtr callb
 #define VERSION_BLINK_PIN       13
 #define IS_PIN_DIGITAL(p)       ((p) >= 2 && (p) <= 19)
 #define IS_PIN_ANALOG(p)        ((p) >= 14 && (p) < 14 + TOTAL_ANALOG_PINS)
-#define IS_PIN_PWM(p)           digitalPinHasPWM(p)
+#define ARDUINO_IS_PIN_PWM(x)   (((x & PIN_USE_MASK) ==  PIN_PWM_GPT) || ((x & PIN_USE_MASK) ==  PIN_PWM_AGT))
+#define ARDUINO_digitalPinHasPWM(p) (ARDUINO_IS_PIN_PWM(getPinCfgs(p, PIN_CFG_REQ_PWM)[0]))
+#define IS_PIN_PWM(p)           ARDUINO_digitalPinHasPWM(p)
 #define IS_PIN_SERVO(p)         (IS_PIN_DIGITAL(p) && (p) - 2 < MAX_SERVOS)
 #define IS_PIN_I2C(p)           ((p) == 18 || (p) == 19)
 #define IS_PIN_SPI(p)           ((p) == SS || (p) == MOSI || (p) == MISO || (p) == SCK)

[zfields]

When you guys land on a desired solution, please remember to port it back to the standard Firmata implementation.

[pgrawehr]

@per1234 I believe this should do the trick. Can you verify, please?

[zfields]

I like the FIRMATA_ prefix. I think that was a good choice. 👍

[per1234]

Looks great. Thanks @pgrawehr!

[pgrawehr]

I'll take a look at StandardFirmata at a later time again.